Bug 214067

Created attachment 403753 [details]
patch

Comment on attachment 403753 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403753&action=review

r=me

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14718
> +        patchpoint->effects.writesLocalState = true;

patchpoint->clobber(RegisterSet::macroScratchRegisters()); is required.

> Source/JavaScriptCore/runtime/VM.cpp:1562
> +        uint64_t* ptr = static_cast<uint64_t*>(fastMalloc(sizeof(uint64_t)));
> +        *ptr = 0;
> +        addResult.iterator->value.second = ptr;

How about using `std::unique_ptr<uint64_t>` & `makeUnique<uint64_t>()`?

> Source/JavaScriptCore/runtime/VM.cpp:1582
> +        fastFree(iter->value.second);

Then, we can remove this.

Comment on attachment 403753 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403753&action=review

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14718
>> +        patchpoint->effects.writesLocalState = true;
> 
> patchpoint->clobber(RegisterSet::macroScratchRegisters()); is required.

Actually I tried to not make it so that we use them. I added AllowMacroScratxh RAII before I refactored the code. I will remove the RAII

>> Source/JavaScriptCore/runtime/VM.cpp:1562
>> +        addResult.iterator->value.second = ptr;
> 
> How about using `std::unique_ptr<uint64_t>` & `makeUnique<uint64_t>()`?

Sounds good

Comment on attachment 403753 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403753&action=review

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:468
> +        *ptr += codeBlock->llintExecuteCounter().m_activeThreshold;

Why inc by m_activeThreshold here and only by 1 in the JITs?

Comment on attachment 403753 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403753&action=review

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:468
>> +        *ptr += codeBlock->llintExecuteCounter().m_activeThreshold;
> 
> Why inc by m_activeThreshold here and only by 1 in the JITs?

this is just a heuristic of how much we've executed. This is actually slightly higher than what we've executed. It's because I don't want to change the LLInt's runtime code. But changing the JITs is trivial, since it's controlled behind a runtime option.

Created attachment 403757 [details]
patch for landing

Comment on attachment 403753 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403753&action=review

>>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:468
>>> +        *ptr += codeBlock->llintExecuteCounter().m_activeThreshold;
>> 
>> Why inc by m_activeThreshold here and only by 1 in the JITs?
> 
> this is just a heuristic of how much we've executed. This is actually slightly higher than what we've executed. It's because I don't want to change the LLInt's runtime code. But changing the JITs is trivial, since it's controlled behind a runtime option.

OK, I understand now.  The detail to note is that this function (llint_loop_osr) is only called once after the loop_hint has been visited m_activeThreshold times.  It is not called on every loop_hint.

Created attachment 403758 [details]
patch for landing

Now skipping this for builtin functions, since we rely on some of those not early returning out of loops for the integrity of data structures we create.

Committed r264105: <https://trac.webkit.org/changeset/264105>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403758 [details].

<rdar://problem/65227843>