Closed Bug 996983 Opened 6 years ago Closed 6 years ago

Make activations and frame iterators threadsafe.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(5 files, 2 obsolete files)

We need to be able to walk the stack in PJS, both for GC scanning over in bug
933313 and for just general quality of life.
Changes the base types to be threadsafe. The derived types like
InterpreterActivation still take a JSContext *.

The compartment_ field is made private and a getter is added to make sure that
asking for the compartment is only done on eligible threads.
Attachment #8407270 - Flags: review?(luke)
Attached patch Part 2: JIT-related changes (obsolete) — Splinter Review
JIT-related parts, pretty straightforward.
Attachment #8407271 - Flags: review?(jdemooij)
Actually push a JitActivation per thread.
Attachment #8407272 - Flags: review?(nmatsakis)
Blocks: 933313
Split up the patches wrong the first time.
Attachment #8407277 - Flags: review?(jdemooij)
Split up the patches wrong the first time.
Attachment #8407278 - Flags: review?(nmatsakis)
Attachment #8407272 - Attachment is obsolete: true
Attachment #8407272 - Flags: review?(nmatsakis)
Attachment #8407271 - Attachment is obsolete: true
Attachment #8407271 - Flags: review?(jdemooij)
Comment on attachment 8407270 [details] [diff] [review]
Part 1: Change activations and friends to use ThreadSafeContext. Non-JIT-related changes

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

::: js/src/vm/Stack-inl.h
@@ +849,5 @@
> +
> +inline JSCompartment *
> +Activation::compartment() const
> +{
> +    MOZ_ASSERT(compartment_->runtimeFromMainThread());

It's a bit unfortunate that this isn't more "typey".  I guess that's hard because JitActivation can be used on or off the main thread.  We could parameterize All The Things with a ContextT that is either JSContext or ThreadSafeContext... but that'd be kindof a lot of hassle.  Another idea is to make a new subclass of ScriptFrameIter called ForkJoinScriptFrameIter (or something).  This new subclass:
 - be the only FrameIter with a public ThreadSafeContext constructor; all the others would take JSContext
 - could either publicly derive ScriptFrameIter and hide the unsafe methods or privately derive ScriptFrameIter and expose the safe methods
This might be kinda nice... what do you think?
(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8407270 [details] [diff] [review]
> Part 1: Change activations and friends to use ThreadSafeContext.
> Non-JIT-related changes
> 
> Review of attachment 8407270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Stack-inl.h
> @@ +849,5 @@
> > +
> > +inline JSCompartment *
> > +Activation::compartment() const
> > +{
> > +    MOZ_ASSERT(compartment_->runtimeFromMainThread());
> 
> It's a bit unfortunate that this isn't more "typey".  I guess that's hard
> because JitActivation can be used on or off the main thread.  We could
> parameterize All The Things with a ContextT that is either JSContext or
> ThreadSafeContext... but that'd be kindof a lot of hassle.  Another idea is
> to make a new subclass of ScriptFrameIter called ForkJoinScriptFrameIter (or
> something).  This new subclass:
>  - be the only FrameIter with a public ThreadSafeContext constructor; all
> the others would take JSContext
>  - could either publicly derive ScriptFrameIter and hide the unsafe methods
> or privately derive ScriptFrameIter and expose the safe methods
> This might be kinda nice... what do you think?

A new iterator class sounds good to me.
Attachment #8407278 - Flags: review?(nmatsakis) → review+
(In reply to Shu-yu Guo [:shu] from comment #7)
> (In reply to Luke Wagner [:luke] from comment #6)
> > Comment on attachment 8407270 [details] [diff] [review]
> > Part 1: Change activations and friends to use ThreadSafeContext.
> > Non-JIT-related changes
> > 
> > Review of attachment 8407270 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/vm/Stack-inl.h
> > @@ +849,5 @@
> > > +
> > > +inline JSCompartment *
> > > +Activation::compartment() const
> > > +{
> > > +    MOZ_ASSERT(compartment_->runtimeFromMainThread());
> > 
> > It's a bit unfortunate that this isn't more "typey".  I guess that's hard
> > because JitActivation can be used on or off the main thread.  We could
> > parameterize All The Things with a ContextT that is either JSContext or
> > ThreadSafeContext... but that'd be kindof a lot of hassle.  Another idea is
> > to make a new subclass of ScriptFrameIter called ForkJoinScriptFrameIter (or
> > something).  This new subclass:
> >  - be the only FrameIter with a public ThreadSafeContext constructor; all
> > the others would take JSContext
> >  - could either publicly derive ScriptFrameIter and hide the unsafe methods
> > or privately derive ScriptFrameIter and expose the safe methods
> > This might be kinda nice... what do you think?
> 
> A new iterator class sounds good to me.

We discussed on IRC and concluded to hide compartment() from Activation. On revisiting this, I decided against this hiding. It's too intrusive and doesn't add much safety to subclass ActivationIterators and FrameIterators, given how many classes there are. After all, it's trivial to get a compartment from any JSObject without any thread safety checks.

We also implicitly agreed that that a static analysis is the way to go over burdening the API anyways. I'm leaving the patch as is.
In fact, I've decided to leave compartment() unchecked, as it is now. It is all
too easy to get a compartment from a JSObject, so adding some checks on the
activation will almost be "thread-safety theater".
Attachment #8409974 - Flags: review?(luke)
Attachment #8407277 - Flags: review?(jdemooij) → review+
(In reply to Shu-yu Guo [:shu] from comment #5)
> Created attachment 8407278 [details] [diff] [review]
> Part 3: Push JitActivations for each PJS thread during ForkJoin

There's a bug in this patch, before the call it leaves result in a default state ("undefined" probably), but it needs to be e.g. zero:

    Value result = JSVAL_ZERO;

The reason is that result denotes the number of arguments to preserve after the call.  The stack walker walks off a cliff unless the value is set properly.

(The bug was in the old code too.)
Attachment #8407270 - Flags: review?(luke) → review+
Comment on attachment 8409974 [details] [diff] [review]
Part 1: Change activations and friends to use ThreadSafeContext. Non-JIT-related changes

Err, this one.
Attachment #8409974 - Flags: review?(luke) → review+
Note, patches 1-3 no longer apply cleanly (in fact must hunks fail to apply, some files are even gone).
Re comment #12: This patch combines patches 1-3 and rebases everything to m-c 180667.

This might need re-review: I had to weed out some uses of JSContext in Ion; looks to me like ThreadSafeContext should be just fine, but what do I know?
Luckily some of the merge failures were caused by my own patches so they were easy fixes.
This fixed the kraken-beat-detection regression on that happened in http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e0679448fd3b&tochange=4b91913320ef

I couldn't reproduce the regression yet, so I couldn't pinpoint the right bug. Is it expected this could fix a regression?
Hm, this was a refactor and should have no perf impact. What caused the regression?
You need to log in before you can comment on or make changes to this bug.