...
Created attachment 429617 [details] Patch
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?
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.
Created attachment 429711 [details] Patch
Created attachment 429713 [details] Patch
Created attachment 429719 [details] Patch
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].
<rdar://problem/78513094>