Closed
Bug 1334927
Opened 6 years ago
Closed 6 years ago
Handle multiple contexts per runtime in the SPS profiler
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(2 files, 1 obsolete file)
16.18 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
7.39 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
SPS assumes in a number of places that the runtime has only one main thread. Fixing this to tolerate multiple threads in a cooperatively threaded runtime doesn't seem too tricky (this is just the JS side of things, though; the profiler will of course need a fair amount of work when the browser itself is multithreaded). Preemptive multithreading will need more (probably substantially more) work to handle.
Attachment #8831568 -
Flags: review?(shu)
Assignee | ||
Comment 1•6 years ago
|
||
This patch adds a AutoProhibitActiveContextChange class which makes things tidier and better separated here and will be useful in later patches for bug 1323066.
Attachment #8831568 -
Attachment is obsolete: true
Attachment #8831568 -
Flags: review?(shu)
Attachment #8831672 -
Flags: review?(shu)
Comment 2•6 years ago
|
||
Comment on attachment 8831672 [details] [diff] [review] patch Review of attachment 8831672 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CompileWrappers.cpp @@ -93,5 @@ > > const void* > CompileRuntime::addressOfActiveJSContext() > { > - return &runtime()->activeContext; Where's the patch that added activeContext? I don't see it in my tree.
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #2) > Comment on attachment 8831672 [details] [diff] [review] > patch > > Review of attachment 8831672 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/CompileWrappers.cpp > @@ -93,5 @@ > > > > const void* > > CompileRuntime::addressOfActiveJSContext() > > { > > - return &runtime()->activeContext; > > Where's the patch that added activeContext? I don't see it in my tree. Bug 1334837 will add JSRuntime::activeContext.
Comment 4•6 years ago
|
||
Comment on attachment 8831672 [details] [diff] [review] patch Review of attachment 8831672 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine to me, but I'm lacking the big picture for how this is intended to be used. ::: js/src/vm/Runtime.h @@ +304,5 @@ > + mozilla::Atomic<JSContext*, mozilla::ReleaseAcquire> activeContext_; > + > + // All contexts participating in cooperative scheduling. All threads other > + // than |activeContext_| are suspended. > + js::ActiveThreadData<js::Vector<JSContext*, 4, js::SystemAllocPolicy>> cooperatingContexts_; I couldn't find ActiveThreadData either. I tried looking for it in one of the other bugs and only found ProtectedData and its subclasses, with ActiveThread not among them. I imagine it's for DEBUG-only checks that ref() is being called from the active thread? That is, the data is always single writer single reader? @@ +307,5 @@ > + // than |activeContext_| are suspended. > + js::ActiveThreadData<js::Vector<JSContext*, 4, js::SystemAllocPolicy>> cooperatingContexts_; > + > + // Count of AutoProhibitActiveContextChange instances on the active context. > + js::ActiveThreadData<size_t> activeContextChangeProhibited; Should have a trailing _ by convention for private fields. @@ +329,5 @@ > + return false; > + } > +#endif > + > + class MOZ_RAII AutoProhibitActiveContextChange AFAICT this class and |activeContextChangeProhibited| field is used for DEBUG-only checks. Is the a reason this class and the field are not DEBUG-only a subsequent patch? The name is a bit confusing, as this doesn't actually prohibit changing the active context. It's more like AutoAssertNoActiveContextChange right now. I don't know future plans though.
Attachment #8831672 -
Flags: review?(shu) → review+
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #4) > ::: js/src/vm/Runtime.h > @@ +304,5 @@ > > + mozilla::Atomic<JSContext*, mozilla::ReleaseAcquire> activeContext_; > > + > > + // All contexts participating in cooperative scheduling. All threads other > > + // than |activeContext_| are suspended. > > + js::ActiveThreadData<js::Vector<JSContext*, 4, js::SystemAllocPolicy>> cooperatingContexts_; > > I couldn't find ActiveThreadData either. I tried looking for it in one of > the other bugs and only found ProtectedData and its subclasses, with > ActiveThread not among them. I imagine it's for DEBUG-only checks that ref() > is being called from the active thread? That is, the data is always single > writer single reader? ActiveThreadData is also added in bug 1334837. Yes, it ensures in debug builds that accesses only occur on the currently active cooperative thread. > @@ +329,5 @@ > > + return false; > > + } > > +#endif > > + > > + class MOZ_RAII AutoProhibitActiveContextChange > > AFAICT this class and |activeContextChangeProhibited| field is used for > DEBUG-only checks. Is the a reason this class and the field are not > DEBUG-only a subsequent patch? > > The name is a bit confusing, as this doesn't actually prohibit changing the > active context. It's more like AutoAssertNoActiveContextChange right now. I > don't know future plans though. Hmm, I can change the check to a MOZ_RELEASE_ASSERT, I think it'd be good to make absolutely sure we don't get unexpected thread switches. None of the places this class is used should be hot.
Comment 6•6 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #5) > (In reply to Shu-yu Guo [:shu] from comment #4) > > ::: js/src/vm/Runtime.h > > @@ +329,5 @@ > > > + return false; > > > + } > > > +#endif > > > + > > > + class MOZ_RAII AutoProhibitActiveContextChange > > > > AFAICT this class and |activeContextChangeProhibited| field is used for > > DEBUG-only checks. Is the a reason this class and the field are not > > DEBUG-only a subsequent patch? > > > > The name is a bit confusing, as this doesn't actually prohibit changing the > > active context. It's more like AutoAssertNoActiveContextChange right now. I > > don't know future plans though. > > Hmm, I can change the check to a MOZ_RELEASE_ASSERT, I think it'd be good to > make absolutely sure we don't get unexpected thread switches. None of the > places this class is used should be hot. Yeah, let's do MOZ_RELEASE_ASSERT.
Assignee | ||
Comment 7•6 years ago
|
||
This followup fixes a place that turned up during testing where the runtime is used to get the context of a thread whose profiling frames are being iterated. Since ProfilingFrameIterator was originally passed the context to iterate over, fixing this just requires keeping track of that context more closely.
Attachment #8833492 -
Flags: review?(shu)
Comment 8•6 years ago
|
||
Comment on attachment 8833492 [details] [diff] [review] followup Review of attachment 8833492 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Stack.cpp @@ +1940,5 @@ > } > > // Extract the stack for the entry. Assume maximum inlining depth is <64 > const char* labels[64]; > + uint32_t depth = entry.callStackAtAddr(cx_->runtime(), jitIter().returnAddressToFp(), labels, 64); While you're here, could you switch 64 to ArrayLength(labels)?
Attachment #8833492 -
Flags: review?(shu) → review+
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1aac89d4610d Handle multiple contexts per runtime in the Gecko profiler, r=shu.
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1aac89d4610d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•