Bug 226207 – Merge all the JIT worklists into a shared worklist

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 429617 [details]
Patch

[Saam Barati]

Comment on attachment 429617 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:13
> +        on uper tiers when all lower tiers have no pending tasks or have exceeded the maximum

uper => upper

> Source/JavaScriptCore/dfg/DFGPlan.cpp:138
> +        : JITPlan(mode, passedCodeBlock)
> +        , m_profiledDFGCodeBlock(profiledDFGCodeBlock)
> +        , m_mustHandleValues(mustHandleValues)
> +        , m_osrEntryBytecodeIndex(osrEntryBytecodeIndex)
> +        , m_compilation(UNLIKELY(m_vm->m_perBytecodeProfiler) ? adoptRef(new Profiler::Compilation(m_vm->m_perBytecodeProfiler->ensureBytecodesFor(m_codeBlock), profilerCompilationKindForMode(mode))) : nullptr)
> +        , m_inlineCallFrames(adoptRef(new InlineCallFrameSet()))
> +        , m_identifiers(m_codeBlock)
> +        , m_weakReferences(m_codeBlock)
> +        , m_transitions(m_codeBlock)

indentation looks off here, but not sure if it's real or it's bugzilla

> Source/JavaScriptCore/dfg/DFGPlan.cpp:169
> +    JITPlan::cancel();

nit: I find it a bit more elegant to define JITPlan as Base in the class definition in the header, and use Base instead of JITPlan in the various places

> Source/JavaScriptCore/heap/Heap.cpp:-1661
> -            setGCDidJIT();

let's now remove this function and anything using `gcDidJITBit`, like handleGCDidJIT, etc

> Source/JavaScriptCore/jit/JITBaselinePlan.cpp:33
> +JITBaselinePlan::JITBaselinePlan(CodeBlock* codeBlock, BytecodeIndex loopOSREntryBytecodeIndex)

nit: I think I'd name this BaselineJITPlan

> Source/JavaScriptCore/jit/JITPlan.cpp:148
> +    MonotonicTime before { };

don't think you need the "{ }"

> Source/JavaScriptCore/jit/JITPlan.h:75
> +    virtual void notifyCompiling();

why virtual? From what I can tell, only one implementation.

> Source/JavaScriptCore/jit/JITWorklist.cpp:79
> +CompilationResult JITWorklist::enqueue(Ref<JITPlan>&& plan)

nit: can this just be a Ref<JITPlan> instead of Ref<JITPlan>&&? That way, this function just takes ownership over the ref, and it can happily still WTFMove it in the CJIT case

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:381
> +    if (codeBlock->jitType() == JITType::BaselineJIT || worklistState == JITWorklist::Compiled) {

why do we need both these conditions? Won't Compiled imply we installed the new code?

[Tadeu Zagallo]

Comment on attachment 429617 [details]
Patch

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

Thanks for the review

>> Source/JavaScriptCore/dfg/DFGPlan.cpp:138
>> +        , m_transitions(m_codeBlock)
> 
> indentation looks off here, but not sure if it's real or it's bugzilla

I agree, but that's what check-webkit-style wanted

>> Source/JavaScriptCore/dfg/DFGPlan.cpp:169
>> +    JITPlan::cancel();
> 
> nit: I find it a bit more elegant to define JITPlan as Base in the class definition in the header, and use Base instead of JITPlan in the various places

Will do

>> Source/JavaScriptCore/heap/Heap.cpp:-1661
>> -            setGCDidJIT();
> 
> let's now remove this function and anything using `gcDidJITBit`, like handleGCDidJIT, etc

👍

>> Source/JavaScriptCore/jit/JITBaselinePlan.cpp:33
>> +JITBaselinePlan::JITBaselinePlan(CodeBlock* codeBlock, BytecodeIndex loopOSREntryBytecodeIndex)
> 
> nit: I think I'd name this BaselineJITPlan

That's a good point, I got hung up on the JIT prefix, but that's better

>> Source/JavaScriptCore/jit/JITPlan.cpp:148
>> +    MonotonicTime before { };
> 
> don't think you need the "{ }"

will update, this was just moved from DFGPlan

>> Source/JavaScriptCore/jit/JITPlan.h:75
>> +    virtual void notifyCompiling();
> 
> why virtual? From what I can tell, only one implementation.

will remove

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:381
>> +    if (codeBlock->jitType() == JITType::BaselineJIT || worklistState == JITWorklist::Compiled) {
> 
> why do we need both these conditions? Won't Compiled imply we installed the new code?

Yes, but I think I ran into issues with this, I'll double check.

[Tadeu Zagallo]

Created attachment 429711 [details]
Patch

[Tadeu Zagallo]

Created attachment 429713 [details]
Patch

[Tadeu Zagallo]

Created attachment 429719 [details]
Patch

[EWS]

Committed r278082 (238161@main): <https://commits.webkit.org/238161@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78513094>