Bug 220718

Summary:

[JSC] FTL::prepareOSREntry can clear OSR entry CodeBlock if it is already invalidated

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

Description Yusuke Suzuki 2021-01-18 17:19:01 PST

[JSC] FTL::prepareOSREntry can clear OSR entry CodeBlock if it is already invalidated

Comment 1 Yusuke Suzuki 2021-01-18 17:20:39 PST

Created attachment 417853 [details]
Patch

Comment 2 Yusuke Suzuki 2021-01-18 17:20:42 PST

<rdar://problem/70527068>

Comment 3 Yusuke Suzuki 2021-01-18 17:21:48 PST

Created attachment 417854 [details]
Patch

Comment 4 Mark Lam 2021-01-18 19:53:26 PST

Comment on attachment 417854 [details]
Patch

r=mer

Comment 5 EWS 2021-01-18 21:00:00 PST

Committed r271596: <https://trac.webkit.org/changeset/271596>

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

Comment 6 Michael Catanzaro 2021-01-19 10:43:42 PST

This introduced:

[621/2299] Building CXX object Source/JavaScriptCore/CMak...criptCore/unified-sources/UnifiedSource-bfc896e1-12.cpp.o
In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-12.cpp:5:
../../Source/JavaScriptCore/dfg/DFGOperations.cpp: In function ‘char* JSC::DFG::tierUpCommon(JSC::VM&, JSC::CallFrame*, JSC::BytecodeIndex, bool)’:
../../Source/JavaScriptCore/dfg/DFGOperations.cpp:3861:20: warning: unused variable ‘entryBlock’ [-Wunused-variable]
 3861 |     if (CodeBlock* entryBlock = jitCode->osrEntryBlock()) {
      |                    ^~~~~~~~~~

I'm not sure if this trivial fix would be correct:

diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
index 85e74d17ece6..45fdcb24f68c 100644
--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
@@ -3858,7 +3858,7 @@ static char* tierUpCommon(VM& vm, CallFrame* callFrame, BytecodeIndex originByte
     } else
         CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("avoiding replacement compile"));
 
-    if (CodeBlock* entryBlock = jitCode->osrEntryBlock()) {
+    if (jitCode->osrEntryBlock()) {
         if (jitCode->osrEntryRetry < Options::ftlOSREntryRetryThreshold()) {
             CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("OSR entry failed, OSR entry threshold not met"));
             jitCode->osrEntryRetry++;

Maybe that condition is no longer needed at all now?

Comment 7 Yusuke Suzuki 2021-01-19 15:25:27 PST

(In reply to Michael Catanzaro from comment #6)
> This introduced:
> 
> [621/2299] Building CXX object
> Source/JavaScriptCore/CMak...criptCore/unified-sources/UnifiedSource-
> bfc896e1-12.cpp.o
> In file included from
> DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-bfc896e1-12.cpp:
> 5:
> ../../Source/JavaScriptCore/dfg/DFGOperations.cpp: In function ‘char*
> JSC::DFG::tierUpCommon(JSC::VM&, JSC::CallFrame*, JSC::BytecodeIndex, bool)’:
> ../../Source/JavaScriptCore/dfg/DFGOperations.cpp:3861:20: warning: unused
> variable ‘entryBlock’ [-Wunused-variable]
>  3861 |     if (CodeBlock* entryBlock = jitCode->osrEntryBlock()) {
>       |                    ^~~~~~~~~~
> 
> I'm not sure if this trivial fix would be correct:
> 
> diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp
> b/Source/JavaScriptCore/dfg/DFGOperations.cpp
> index 85e74d17ece6..45fdcb24f68c 100644
> --- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
> +++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
> @@ -3858,7 +3858,7 @@ static char* tierUpCommon(VM& vm, CallFrame*
> callFrame, BytecodeIndex originByte
>      } else
>          CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("avoiding
> replacement compile"));
>  
> -    if (CodeBlock* entryBlock = jitCode->osrEntryBlock()) {
> +    if (jitCode->osrEntryBlock()) {
>          if (jitCode->osrEntryRetry < Options::ftlOSREntryRetryThreshold()) {
>              CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("OSR entry
> failed, OSR entry threshold not met"));
>              jitCode->osrEntryRetry++;
> 
> Maybe that condition is no longer needed at all now?

That condition is required.

if (jitCode->osrEntryBlock()) {

is the right change. I'll land it as an unreviewed patch.

Comment 8 Yusuke Suzuki 2021-01-19 15:27:15 PST

Committed r271624: <https://trac.webkit.org/changeset/271624>