Closed
Bug 1178834
Opened 9 years ago
Closed 9 years ago
Always lazy link ion code
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(1 file, 2 obsolete files)
28.38 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
We currently only lazy link for code that gets recompiled and is still on the stack. Remove this restriction and always lazy link.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee: nobody → hv1989
Attachment #8627768 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•9 years ago
|
||
review ping?
Comment 3•9 years ago
|
||
Comment on attachment 8627768 [details] [diff] [review] always-lazy-linking Clearing review flag for now per our IRC conversation.
Attachment #8627768 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•9 years ago
|
||
Adjusted patch to save pendingIonBuilder in baseline. Passes jittests but gonna double check everything tonight for correctness.
Assignee | ||
Comment 5•9 years ago
|
||
Now the timeouts using the "onIonCompilation" are also fixed.
Attachment #8627768 -
Attachment is obsolete: true
Attachment #8641024 -
Attachment is obsolete: true
Attachment #8641534 -
Flags: review?(jdemooij)
Comment 6•9 years ago
|
||
Comment on attachment 8641534 [details] [diff] [review] always-lazy-linking Review of attachment 8641534 [details] [diff] [review]: ----------------------------------------------------------------- Nice! The patch file is a bit weird (some files are duplicated) and I can't add this comment to BaselineJIT.cpp, but in BaselineScript::Destroy, can we assert we don't have a pending builder? I think we should have finished all off-thread compilations at that point. ::: js/src/jit/BaselineIC.cpp @@ +137,5 @@ > return true; > } > > if (isLoopEntry) { > + // Check if the jitcode still need to get linked and do this Nit: needs @@ +145,5 @@ > + if (!script->hasIonScript()) > + return true; > + if (script->ionScript()->osrPc() != pc) > + return true; > + } Can you move this code to CanEnterAtBranch? I think that's nice because both CanEnter and CanEnterAtBranch will have similar LazyLink code then, and I think you can remove the osrPc check because CanEnterAtBranch already checks that too. ::: js/src/jit/Ion.cpp @@ +466,5 @@ > void > jit::FinishOffThreadBuilder(JSContext* cx, IonBuilder* builder) > { > // Clean the references to the pending IonBuilder, if we just finished it. > + if (builder->script()->hasBaselineScript() && Aren't we guaranteed to have a BaselineScript here? Would be nice if we could remove this check to make this clear. @@ -1785,5 @@ > JitCompartment* ion = cx->compartment()->jitCompartment(); > if (!ion) > return; > > - LifoAlloc* debuggerAlloc = cx->new_<LifoAlloc>(TempAllocator::PreferredLifoChunkSize); Nice code removal :) ::: js/src/jsscript.h @@ +965,5 @@ > js::HeapPtrFunction function_; > js::HeapPtrObject enclosingStaticScope_; > > + /* > + * Information attached by Ion for sequential mode execution. Nit: remove the "for sequential execution" part, it's no longer relevant since the PJS removal. @@ +974,3 @@ > js::jit::IonScript* ion; > + > + /* Information attached by Baseline for sequential mode execution. */ Same here.
Attachment #8641534 -
Flags: review?(jdemooij) → review+
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3bbd0d929128
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•