Closed
Bug 1042729
Opened 7 years ago
Closed 7 years ago
Clean the MBasicBlock list of resume point.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: nbp, Assigned: nbp)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
6.40 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
11.00 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
9.15 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
8.05 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
5.45 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
9.69 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
Currently each basic block keeps a list of resume points which are attached to the basic block or attached to an instruction. This was done for discarding the uses hold by orphan resume points after the restartLoop phase of IonBuilder (Bug 865153). With the arrival of the escape analysis and other similar optimizations, we are now able to remove effectful instructions to the extend that we can recover their instructions. As we remove these instruction we should also remove the resume points which are associated to these instructions (Bug 1040940). If we do not do so, we might have resume points which are visible on the use list of instructions while not being used for any snapshot during the lowering. Worse, some instructions seems to be sharing the same resume points, which is unfortunate when we are trying to remove when an instruction which is holding it. I am opening this bug to add some sanity back to the way we handle the list of resume points attached on the blocks. We should be able to ensure that the list of resume points only contains valid resume points and that resume points are only attached to one instruction only.
Assignee | ||
Comment 1•7 years ago
|
||
This patch adds a MResumePoint::Copy function to replace the implicit reuse of the MStrart resume point, or the basic block entry resume point. Doing so will indeed consume more memory, but with the up-comming patches, we should be able to remove the MInstruction pointer from ResumePoint which will surely overcome the down-side of allocating more resume point in the current patch.
Attachment #8461487 -
Flags: review?(hv1989)
Assignee | ||
Comment 2•7 years ago
|
||
Add the graph invariant to make sure that all resume points which are in the list of resume points are live, and that all instructions are holding their own resume point if they have one.
Attachment #8461492 -
Flags: review?(hv1989)
Updated•7 years ago
|
Attachment #8461492 -
Flags: review?(hv1989) → review+
Updated•7 years ago
|
Attachment #8461487 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 3•7 years ago
|
||
This patch prevent the last case of orphan resume point which are used for inlining. This patch do 2 things: - Re-attach the resume point to the new block, when the resume point is used as the entry point of the new block. - Discard the resume point if we do not used it for inlining, and discard all its uses.
Attachment #8462530 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 4•7 years ago
|
||
This patch reference precisely the last case of resume point which is not precisely named. The outer resume point is used for holding the state of the stack of Baseline before the call. Outer resume points are used to be hold as the caller_ resume point in all the inlined basic blocks. As this is the last case, we can also remove the processing of the list of resume point when we are discarding all the resume points, and only assert that there is no more resume points as soon as we have removed all instructions.
Attachment #8462535 -
Flags: review?(hv1989)
Assignee | ||
Comment 5•7 years ago
|
||
Move the list of resume points to be a Debug-only thing that we use to assert that we properly remove blocks, and to verify the graph coherency.
Attachment #8462536 -
Flags: review?(hv1989)
Assignee | ||
Updated•7 years ago
|
Comment 6•7 years ago
|
||
Comment on attachment 8462530 [details] [diff] [review] Part 3 - Ensure priorResumePoints are re-attached to the new BasicBlock or discarded. Review of attachment 8462530 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIRGraph.cpp @@ +733,5 @@ > // correctly removing the resume point if any. > MOZ_ASSERT(ins->block() == this); > > MResumePoint *rp = ins->resumePoint(); > if (refType & RefType_DiscardResumePoint && rp) nit: pre-existing: man, I wish there was some extra parens here, just for sanity's sake. ::: js/src/jit/MIRGraph.h @@ +187,5 @@ > resumePoints_.pushFront(resume); > } > > + // Discard pre-allocated resume point. > + void discardPreAllocatedResumePoint(MResumePoint *resume) { Is there some reason why this function doesn't take a ReferencesType defaulted to RefType_Default, and pass that to discardResumePoint()? That would clean up the caller in NewWithresumePoint a little, maybe. I guess it's up to you.
Attachment #8462530 -
Flags: review?(efaustbmo) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8462535 [details] [diff] [review] Part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise. Review of attachment 8462535 [details] [diff] [review]: ----------------------------------------------------------------- Doesn't this get tracked with: "MBasicBlock::callerResumePoint()"?
Attachment #8462535 -
Flags: review?(hv1989)
Updated•7 years ago
|
Attachment #8462536 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #7) > Comment on attachment 8462535 [details] [diff] [review] > Part 4 - Keep a reference to outer resume point on basic blocks, and make > discarding resume point precise. > > Review of attachment 8462535 [details] [diff] [review]: > ----------------------------------------------------------------- > > Doesn't this get tracked with: "MBasicBlock::callerResumePoint()"? Yes, the outer resume point is referenced multiple times by all the resume points which are part of the inlined function. No, and it would not make sense knowing to track it as part of the inline function knowing that the BasicBlock has a CompileInfo which does not correspond to the right function. So the outerResumePoint block should be located on a basic block which compileInfo tragets the right script otherwise we might miss observable operands of the resume points.
Comment 9•7 years ago
|
||
Comment on attachment 8462535 [details] [diff] [review] Part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise. Review of attachment 8462535 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding to Jan, since I'm not certain enough. Looks good or makes sense for me, though.
Attachment #8462535 -
Flags: review?(jdemooij)
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Eric Faust [:efaust] from comment #6) > > + // Discard pre-allocated resume point. > > + void discardPreAllocatedResumePoint(MResumePoint *resume) { > > Is there some reason why this function doesn't take a ReferencesType > defaulted to RefType_Default, and pass that to discardResumePoint()? That > would clean up the caller in NewWithresumePoint a little, maybe. I guess > it's up to you. The goal was to make it clear that discarding resume point is not something that you do every day, only when you have pre-allocated it and not yet attached it to any instruction / basic block yet. Otherwise resume points are discarded when the instruction / basic block is discarded.
Comment 11•7 years ago
|
||
Comment on attachment 8462535 [details] [diff] [review] Part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise. Review of attachment 8462535 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIRGraph.cpp @@ +797,5 @@ > // We only discard operands and flag the instruction as discarded as the > // resume points are exepected to be removed separately. Also we do not > // assert that we have no uses as block might be removed in reverse post > // order. > + discardReferencesTo(*iter, RefType_DefaultNoAssert); Nit: update the comment (the "resume points are expected to be removed separately" part is no longer true).
Attachment #8462535 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b3511fb6b547 https://hg.mozilla.org/integration/mozilla-inbound/rev/6196ec73ac9a https://hg.mozilla.org/integration/mozilla-inbound/rev/4ecf21c3c4a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/749e28d0377e https://hg.mozilla.org/integration/mozilla-inbound/rev/2f56127783b9 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d35b006a2f6
Comment 13•7 years ago
|
||
Backed out for mochitest assertions: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2d35b006a2f6 https://tbpl.mozilla.org/php/getParsedLog.php?id=45242154&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=45242108&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/71a55df665e4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e198397b8a1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0b277b8255 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/208cf26c5af8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9247db062c
Assignee | ||
Comment 14•7 years ago
|
||
I have a test case for this issue, the only issue which explain why I still haven't included the test case is that it is still made of 283 lines. (used to be ~60k) This patch discard the resume point even if the MGetPropertyCache is not interesting for the inlining rule, and that it is not returned there.
Attachment #8476665 -
Flags: review?(efaustbmo)
Comment 15•7 years ago
|
||
Comment on attachment 8476665 [details] [diff] [review] part 3.5 - Discard the resume point if getInlineableGetPropertyCache does not return the MGetPropertyCache. Review of attachment 8476665 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense to me. ::: js/src/jit/IonBuilder.cpp @@ +4299,5 @@ > + MGetPropertyCache *operator->() { > + return get(); > + } > + operator bool() { > + return get(); is this strictly necessary? I know why it's here, and it's cute, but I'm wondering if it's really justified.
Attachment #8476665 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8476665 [details] [diff] [review] part 3.5 - Discard the resume point if getInlineableGetPropertyCache does not return the MGetPropertyCache. Review of attachment 8476665 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.cpp @@ +4299,5 @@ > + MGetPropertyCache *operator->() { > + return get(); > + } > + operator bool() { > + return get(); I guess we can live without it as the only use case is from the condition in IonBuilder::inlineCallsite.
Assignee | ||
Comment 17•7 years ago
|
||
Based on some last pushes to Try, the JQuery error is likely the same one as the one I reproduced with the TypeScript compiler from which I am trying automatically to extract a small test case. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c3a8e9dd40b7 The build error in the above was a missing include in the other set of patches. The build above does not reproduce the mochitest-3 errors, as seen in comment 13.
Assignee | ||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a48a86bf2ef https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9a6d2b3f67 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8802bd12fb2 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5f711e25f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/26dc1dce0e8f I've folded part 3.5 into part 3, as it modifies most of part 3.
https://hg.mozilla.org/mozilla-central/rev/9a48a86bf2ef https://hg.mozilla.org/mozilla-central/rev/fa9a6d2b3f67 https://hg.mozilla.org/mozilla-central/rev/c8802bd12fb2 https://hg.mozilla.org/mozilla-central/rev/9e5f711e25f7 https://hg.mozilla.org/mozilla-central/rev/26dc1dce0e8f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 20•7 years ago
|
||
I'm seeing this assertion failure: Assertion failure: resumePointsEmpty(), at js/src/jit/MIRGraph.cpp:905 when running run-typescript.js in js/src/octane. It works at edfcd8993e42 and fails at 26dc1dce0e8f.
Comment 21•6 years ago
|
||
Comment on attachment 8461487 [details] [diff] [review] Part 1 - Make resume point unique to each instruction. >+ check->setResumePoint(MResumePoint::Copy(alloc(), current->entryResumePoint())); I crashed here today... >+MResumePoint * >+MResumePoint::Copy(TempAllocator &alloc, MResumePoint *src) >+{ >+ MResumePoint *resume = new(alloc) MResumePoint(src->block(), src->pc(), >+ src->caller(), src->mode()); >+ // Copy the operands from the original resume point, and not from the >+ // current block stack. >+ if (!resume->operands_.init(alloc, src->stackDepth())) >+ return nullptr; ... presumably due to this failing for some reason.
Assignee | ||
Comment 22•6 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #21) > Comment on attachment 8461487 [details] [diff] [review] > Part 1 - Make resume point unique to each instruction. > > >+ check->setResumePoint(MResumePoint::Copy(alloc(), current->entryResumePoint())); > I crashed here today... Nice catch, would you be interested in making a patch for fixing it?
You need to log in
before you can comment on or make changes to this bug.
Description
•