[webkit-reviews] review granted: [Bug 226207] Merge all the JIT worklists into a shared worklist : [Attachment 429617] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 226207: Merge all the JIT worklists into a shared worklist
https://bugs.webkit.org/show_bug.cgi?id=226207

Attachment 429617: Patch

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




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 429617
  --> https://bugs.webkit.org/attachment.cgi?id=429617
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_codeBlo
ck), 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?