Do not suppress GC while recovering instruction under bailouts.

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

unspecified
mozilla36
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Created attachment 8506195 [details]
Test case to exercise GCs during bailouts.

Currently, when we are doing a bailout, we suppress all GCs.  The reason being that we are moving from one stack representation to another.  On the other hand, DCE and Scalar Replacement are capable of moving allocation to recover instructions.

When recover instruction are being recovered during the bailout, all allocations of the recover instructions are done under the same AutoSuppressGC.  We should move the AutoSuppressGC after the recovery of the instructions.

Doing so implies being able to mark the bailing stack which is being recovered.
Created attachment 8506932 [details] [diff] [review]
Only remove instructions from the JitActivation after the bailout.

This patch ensure that we keep the RInstructionResults referenced by the
JitActivation. Not copying the vector to the stack implies that we do not
have to add an additional way of marking these results.

Thus, we explicit the fact that we want to get rid of the result which is
always stored on the JitActivation in case of a bailout.
Attachment #8506932 - Flags: review?(jdemooij)
Created attachment 8506941 [details] [diff] [review]
Mark bailout frames.

During a bailout, we have no Safepoint for marking the stack efficiently,
instead we fallback on iterating the frame with a SnapshotIterator.  When we
are tracing an allocation we filter the allocations which are not markable
and give the rest to the GC.

As, a moving GC can be used, we need to feedback any modification made to
the pointers into the memory referenced by the RValueAllocation. To handle
this case, I added a writeAllocationValuePayload function which does the
opposite as allocationValue, such as the traceAllocation function can write
back the modification to the memory.
Attachment #8506941 - Flags: review?(jdemooij)
Created attachment 8507018 [details] [diff] [review]
Handle potential invalidation of the bailing frame.

This patch handle a tricky corner case, which is that a GC can invalidate
the code of the frame which is currently being converted into a baseline
frame. We need to handle the reference counter properly such as (1) the
code is not discarded while we are doing a bailout, and (2) the code is not
leaked once the bailout is completed.
Attachment #8507018 - Flags: review?(jdemooij)
Created attachment 8507046 [details] [diff] [review]
No longer suppress GC for the evaluation of recover instructions.

(not asking for review yet, waiting from treeherder feedback)

This patch is the last one, it enables GCs until we start recovering the
content of the Bailout frames. Thus, all uses of recover instruction can now
trigger a GC as any other object allocation.
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Created attachment 8507046 [details] [diff] [review]
> No longer suppress GC for the evaluation of recover instructions.
> 
> (not asking for review yet, waiting from treeherder feedback)

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ca7594a72335
Attachment #8507046 - Flags: review?(jdemooij)

Updated

4 years ago
Attachment #8506932 - Flags: review?(jdemooij) → review+
Comment on attachment 8506941 [details] [diff] [review]
Mark bailout frames.

Review of attachment 8506941 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonFrames.cpp
@@ +984,5 @@
> +
> +    // We have to mark the list of actual arguments, as only formal arguments
> +    // are represented in the Snapshot.
> +    if (CalleeTokenIsFunction(layout->calleeToken()))
> +        MarkActualArguments(trc, frame);

This needs the fix from bug 1083716 right?
Attachment #8506941 - Flags: review?(jdemooij) → review+

Updated

4 years ago
Attachment #8507018 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #6)
> Comment on attachment 8506941 [details] [diff] [review]
> Mark bailout frames.
> 
> Review of attachment 8506941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/IonFrames.cpp
> @@ +984,5 @@
> > +
> > +    // We have to mark the list of actual arguments, as only formal arguments
> > +    // are represented in the Snapshot.
> > +    if (CalleeTokenIsFunction(layout->calleeToken()))
> > +        MarkActualArguments(trc, frame);
> 
> This needs the fix from bug 1083716 right?

Indeed, I will remove the condition.
Comment on attachment 8507046 [details] [diff] [review]
No longer suppress GC for the evaluation of recover instructions.

Review of attachment 8507046 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #8507046 - Flags: review?(jdemooij) → review+
(In reply to Wes Kocher (:KWierso) from comment #10)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/367d8d88c2cb for
> spidermonkey test failures:
> 
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3178653&repo=mozilla-inbound

I have no idea what is going on, I cannot reproduce locally, and I cannot reproduce on try (with exactly the same changesets).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=41119d516a0c

I re-triggered it multiple times in case this is an intermittent but I have no idea why for the moment.
(In reply to Wes Kocher (:KWierso) from comment #10)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/367d8d88c2cb for
> spidermonkey test failures:
> 
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3178653&repo=mozilla-inbound

By the way, why are we making x64 builds under the category "Build:x86 linux32 linux" ?!
This assertion can only exists if we are packing the values, which is only done on 64 bit builds.
  Assertion failure: (objBits >> 47) == 0
Flags: needinfo?(kwierso)
Created attachment 8509517 [details] [diff] [review]
fixup - Mark bailout frames.

This is the modification that I will merge into the previous patch. This
modification ensure that in case of a TYPED_REG / TYPED_STACK on x64, we are
not writting the tag back into memory.

In case of TYPED_*, we are expecting to load the unboxed value from memory
and use this unboxed value to reconstruct a JSValue.  The assertion is a
sanity assertion which ensure that the pointer is not corrupted.

The UNTYPED_* variant use the raw bits on x64, as opposed to the TYPED_*
case, as the tag is part of what we expect to read from memory.
Attachment #8509517 - Flags: review?(jdemooij)
Comment on attachment 8509517 [details] [diff] [review]
fixup - Mark bailout frames.

Review of attachment 8509517 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch.
Attachment #8509517 - Flags: review?(jdemooij) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> (In reply to Wes Kocher (:KWierso) from comment #10)
> > Backed out in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/367d8d88c2cb for
> > spidermonkey test failures:
> > 
> > https://treeherder.mozilla.org/ui/logviewer.
> > html#?job_id=3178653&repo=mozilla-inbound
> 
> By the way, why are we making x64 builds under the category "Build:x86
> linux32 linux" ?!
> This assertion can only exists if we are packing the values, which is only
> done on 64 bit builds.
>   Assertion failure: (objBits >> 47) == 0

No clue, sorry.
Flags: needinfo?(kwierso)

Comment 19

4 years ago
appears to have caused a regression (-19.2%) on Octane-mandreel on machine 11 on AWFY.

Change from AWFY: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1fe040127ebd&tochange=0f76f410b03b

Updated

4 years ago
Depends on: 1087948
You need to log in before you can comment on or make changes to this bug.