Closed
Bug 996983
Opened 10 years ago
Closed 10 years ago
Make activations and frame iterators threadsafe.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: shu, Assigned: shu)
References
Details
Attachments
(5 files, 2 obsolete files)
20.86 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
8.08 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
4.44 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
20.83 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
36.02 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
JIT-related parts, pretty straightforward.
Attachment #8407271 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•10 years ago
|
||
Actually push a JitActivation per thread.
Attachment #8407272 -
Flags: review?(nmatsakis)
Assignee | ||
Comment 4•10 years ago
|
||
Split up the patches wrong the first time.
Attachment #8407277 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•10 years ago
|
||
Split up the patches wrong the first time.
Attachment #8407278 -
Flags: review?(nmatsakis)
Assignee | ||
Updated•10 years ago
|
Attachment #8407272 -
Attachment is obsolete: true
Attachment #8407272 -
Flags: review?(nmatsakis)
Assignee | ||
Updated•10 years ago
|
Attachment #8407271 -
Attachment is obsolete: true
Attachment #8407271 -
Flags: review?(jdemooij)
Comment 6•10 years ago
|
||
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?
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8407278 -
Flags: review?(nmatsakis) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8407277 -
Flags: review?(jdemooij) → review+
Comment 10•10 years ago
|
||
(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.)
Updated•10 years ago
|
Attachment #8407270 -
Flags: review?(luke) → review+
Comment 11•10 years ago
|
||
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+
Comment 12•10 years ago
|
||
Note, patches 1-3 no longer apply cleanly (in fact must hunks fail to apply, some files are even gone).
Comment 13•10 years ago
|
||
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?
Assignee | ||
Comment 14•10 years ago
|
||
Luckily some of the merge failures were caused by my own patches so they were easy fixes.
Assignee | ||
Comment 15•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b2591a326d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3de709d63355 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3ea6cb5afe
Comment 16•10 years ago
|
||
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?
Assignee | ||
Comment 17•10 years ago
|
||
Hm, this was a refactor and should have no perf impact. What caused the regression?
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b7b2591a326d https://hg.mozilla.org/mozilla-central/rev/3de709d63355 https://hg.mozilla.org/mozilla-central/rev/ed3ea6cb5afe
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•