Closed
Bug 777537
Opened 12 years ago
Closed 12 years ago
JM/IonMonkey: Ensure that CompilerOuput status mirror validity without output pointers
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: nbp, Assigned: nbp)
References
Details
(Whiteboard: [ion:p1:fx18])
Attachments
(2 files)
31.54 KB,
patch
|
bhackett1024
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
785 bytes,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
The current implementation CompilerOutput can still own a dandling pointer to a compiler output which has been invalidated. The current implementation is not supposed to use this pointer because we ensure that everything is valid before making any changes. Making the CompilerOutput mirror the validity would be nicer. This imply to modify addPendingRecompiles(cx, script, pc) to reuse the current CompilerOutputs instead of forging new one from scratch. With such modifications we can ensure that a compilation output can be invalidated only once and thus remove the output pointer because the script is already holding it.
Updated•12 years ago
|
Whiteboard: [ion:t]
Assignee | ||
Comment 1•12 years ago
|
||
Add a constraint index on each result of a compilation. This index correspond to the CompilerOutput structure which identify the result of one compilation in a JSScript. This patch definitively fix unrelated invalidation. Currently this patch is block by some marking issue in IonMonkey, but a subset of it can be used for mozilla-central.
Attachment #651002 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1) > Currently this patch is block by some marking issue in IonMonkey, but a > subset of it can be used for mozilla-central. Update: it works fine against the HEAD of IonMonkey. & Rename the Title to mention that this Bug targets both IonMonkey and central.
Status: NEW → ASSIGNED
Summary: IonMonkey: Ensure that CompilerOuput status mirror validity without output pointers → JM/IonMonkey: Ensure that CompilerOuput status mirror validity without output pointers
Updated•12 years ago
|
Whiteboard: [ion:t] → [ion:p1:fx18]
Comment 3•12 years ago
|
||
Comment on attachment 651002 [details] [diff] [review] Ensure validity of CompilerOutput without a dandling pointer. Review of attachment 651002 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +1231,5 @@ > > +#if defined(DEBUG) && (defined(JS_CPU_X64) || defined(JS_CPU_X86)) > + // No-op used to index the LIR instructions in IonGraph output. > + masm.store16(Imm32(iter->id()), Address(StackPointer, -8)); > +#endif What is this change for? ::: js/src/ion/IonCode.h @@ +206,5 @@ > > // Number of times this function has tried to call a non-IM compileable function > uint32 slowCallCount; > > + types::RecompileInfo constraintIndex_; Needs a comment, and a better name (recompileInfo_?) ::: js/src/jsinfer.cpp @@ +2201,5 @@ > + mjit::JITScript *jit = script->getJIT((bool) constructing, (bool) barriers); > + if (!jit) > + continue; > + > + for (unsigned int c = 0; c < jit->nchunks; c++) { This needs to only invalidate the chunk containing pc, as in the original code. ::: js/src/methodjit/Compiler.cpp @@ +1034,5 @@ > > if (desc.chunk) { > + // Look for the CompilerOutput of the chunk and invalidate it. > + types::TypeCompartment &types = cx->compartment->types; > + desc.chunk->constraintIndex.compilerOutput(types)->invalidate(); Can you put this logic inside JITScript::destroyChunk itself? Having to call both destroyChunk and invalidate() together is error prone.
Attachment #651002 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #3) > Comment on attachment 651002 [details] [diff] [review] > Ensure validity of CompilerOutput without a dandling pointer. > > Review of attachment 651002 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/ion/CodeGenerator.cpp > @@ +1231,5 @@ > > > > +#if defined(DEBUG) && (defined(JS_CPU_X64) || defined(JS_CPU_X86)) > > + // No-op used to index the LIR instructions in IonGraph output. > > + masm.store16(Imm32(iter->id()), Address(StackPointer, -8)); > > +#endif > > What is this change for? This is mostly for tracking issues in debug builds, this is supposed to be a no-op because we should never look below the stack pointer (due to an ARM restriction). So we can safely erase the space below the stack pointer on arch other than ARM to track the LIR index listed in IonGraph output. This is unrelated to the current patch. > ::: js/src/ion/IonCode.h > @@ +206,5 @@ > > > > // Number of times this function has tried to call a non-IM compileable function > > uint32 slowCallCount; > > > > + types::RecompileInfo constraintIndex_; > > Needs a comment, and a better name (recompileInfo_?) I feel like the constraint mechanism is orthogonal to the type inference, and that we should move it out-side the TypeCompartment. The RecompileInfo is only used to index a compiler output in the list of constrained outputs. This explain why I would prefer to do the opposite renaming. But in the mean time I will apply your nit and this feeling can potentially be part of another bug later.
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 651002 [details] [diff] [review] Ensure validity of CompilerOutput without a dandling pointer. ionmonkey: https://hg.mozilla.org/projects/ionmonkey/rev/cfc77da79f9f inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3efa824966
Attachment #651002 -
Flags: checkin+
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1c3efa824966
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 9•12 years ago
|
||
This broke non-methodjit builds : js/src/jsinfer.h:1021: error: 'mjit' has not been declared js/src/jsinfer.h:1021: error: ISO C++ forbids declaration of 'JITScript' with no type js/src/jsinfer.h:1021: error: expected ';' before '*' token See http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/382/steps/build/logs/stdio Dunno what's the best fix here, put the mjit namespace declaration at the beginning of jsinfer.h outside #ifdef JS_METHODJIT or add more #ifdefs to the decl of ::mjit() and callers. I'd go for the first solution, anyone has an advice on it ?
Comment 10•12 years ago
|
||
This fixes that build failure for me on OpenBSD/sparc64. The build now stumbles upon gcc 4.2 (see https://bugzilla.mozilla.org/show_bug.cgi?id=777174#c18) but that's another issue.
Attachment #653175 -
Flags: review?(bhackett1024)
Updated•12 years ago
|
Attachment #653175 -
Flags: review?(bhackett1024) → review+
Comment 12•12 years ago
|
||
The bug number for this commit was typoed, causing the merge tool to close the wrong bug (and future hg blame to be wrong) :-( https://hg.mozilla.org/mozilla-central/rev/9b6f5ce1065a
Comment 13•12 years ago
|
||
Doh! My bad, sorry..
You need to log in
before you can comment on or make changes to this bug.
Description
•