Bug 220718 - [JSC] FTL::prepareOSREntry can clear OSR entry CodeBlock if it is already invalidated
Summary: [JSC] FTL::prepareOSREntry can clear OSR entry CodeBlock if it is already inv...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-18 17:19 PST by Yusuke Suzuki
Modified: 2021-01-19 15:27 PST (History)
8 users (show)

See Also:

Attachments
Patch (5.41 KB, patch)
2021-01-18 17:20 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (4.67 KB, patch)
2021-01-18 17:21 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>