Closed
Bug 1359000
Opened 5 years ago
Closed 5 years ago
Overhaul ThreadInfo and PseudoStack
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(7 files)
1.23 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
4.02 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
7.55 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
9.84 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
14.00 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
55.91 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
30.86 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
There is room for improvement in how ThreadInfo and PseudoStack work.
![]() |
Assignee | |
Comment 1•5 years ago
|
||
Attachment #8860871 -
Flags: review?(mstange)
![]() |
Assignee | |
Updated•5 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•5 years ago
|
||
This patch moves the manual polling up into the preceding loops, which is a better place for it.
Attachment #8860872 -
Flags: review?(mstange)
![]() |
Assignee | |
Comment 3•5 years ago
|
||
"pseudoStack" more closely matches the type name, and is more specific.
Attachment #8860873 -
Flags: review?(mstange)
![]() |
Assignee | |
Comment 4•5 years ago
|
||
Markus, you reluctantly r+'d a similar patch in bug 1347795. I'm hopeful that the performance impact of this will be minimal and that other later changes will more than make up for any degradation here. This change really makes the following patches *much* simpler.
Attachment #8860874 -
Flags: review?(mstange)
![]() |
Assignee | |
Comment 5•5 years ago
|
||
Currently a reference to each thread's PseudoStack is stored in tlsPseudoStack. This patch changes the TLS reference to refer to the enclosing ThreadInfo instead. This allows profiler_clear_js_context() to access the current thread's ThreadInfo via TLs, rather than searching with FindLiveThreadInfo(). The patch also encapsulates the TLS within a new class called TLSInfo. This class allows access to the PseudoStack without protection from gPSMutex, but access to the enclosing ThreadInfo requires a PSLockRef. This maintains the current access regime.
Attachment #8860879 -
Flags: review?(mstange)
![]() |
Assignee | |
Comment 6•5 years ago
|
||
PseudoStack is misnamed: it contains the pseudo-stack plus other stuff that is accessed via TLS. This patch moves the non-pseudo-stack parts of PseudoStack into a new type called RacyThreadInfo, which is a subclass of PseudoStack. The patch also capitalizes the first letter of the names of methods that it moves. This means that PseudoStack is now accurately named. Also non-pseudo-stack parts are now no longer visible outside the profiler, which is nice.
Attachment #8860880 -
Flags: review?(mstange)
![]() |
Assignee | |
Comment 7•5 years ago
|
||
None of the accesses to these fields occur in hot operations, so it's reasonable to do them with gPSMutex held. As a result, mJSSampling doesn't need to be Atomic<>, and mContext's lack of Atomic-ness is no longer a cause for concern. This required adding an extra field, mJSContext, to TickSample.
Attachment #8860881 -
Flags: review?(mstange)
![]() |
Assignee | |
Updated•5 years ago
|
Blocks: profiler-cleanup
Depends on: 1358074
Updated•5 years ago
|
Attachment #8860871 -
Flags: review?(mstange) → review+
Updated•5 years ago
|
Attachment #8860872 -
Flags: review?(mstange) → review+
Updated•5 years ago
|
Attachment #8860873 -
Flags: review?(mstange) → review+
Comment 8•5 years ago
|
||
Comment on attachment 8860874 [details] [diff] [review] (part 4) - De-inline profiler_call_{enter,exit} Review of attachment 8860874 [details] [diff] [review]: ----------------------------------------------------------------- I'm still very reluctant to r+ this, but you make a convincing argument. And the fact that one needs to recompile the world when touching PseudoStack.h has long been a thorn in my side.
Attachment #8860874 -
Flags: review?(mstange) → review+
Updated•5 years ago
|
Attachment #8860879 -
Flags: review?(mstange) → review+
Comment 9•5 years ago
|
||
Comment on attachment 8860880 [details] [diff] [review] (part 6) - Split off RacyThreadInfo from PseudoStack Review of attachment 8860880 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/ThreadInfo.h @@ +18,5 @@ > + > +// This class contains the info for a single thread that is accessible without > +// protection from gPSMutex in platform.cpp. Because there is no external > +// protection against data races, it must provide internal protection. Hence > +// the "Racy" prefix. Aren't things that can be accessed from multiple threads without protection usually called threadsafe? @@ +54,5 @@ > + > + return n; > + } > + > + void AddPendingMarker(const char* aMarkerStr, ProfilerMarkerPayload* aPayload, This is only called on-thread, I think. Add a comment?
Attachment #8860880 -
Flags: review?(mstange) → review+
Updated•5 years ago
|
Attachment #8860881 -
Flags: review?(mstange) → review+
Comment 10•5 years ago
|
||
Comment on attachment 8860880 [details] [diff] [review] (part 6) - Split off RacyThreadInfo from PseudoStack Review of attachment 8860880 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/ThreadInfo.cpp @@ +23,5 @@ > void* aStackTop) > : mName(strdup(aName)) > , mThreadId(aThreadId) > , mIsMainThread(aIsMainThread) > + , mRacyInfo(mozilla::WrapNotNull(new RacyThreadInfo())) Could this be a regular field now?
Updated•5 years ago
|
status-firefox55:
--- → affected
status-firefox57:
affected → ---
![]() |
Assignee | |
Comment 11•5 years ago
|
||
> > +// This class contains the info for a single thread that is accessible without
> > +// protection from gPSMutex in platform.cpp. Because there is no external
> > +// protection against data races, it must provide internal protection. Hence
> > +// the "Racy" prefix.
>
> Aren't things that can be accessed from multiple threads without protection
> usually called threadsafe?
I have seen that once or twice and found it highly confusing! That naming scheme seems entirely backwards to me. "Racy" suggests "be careful!" which is appropriate. "Threadsafe" suggests "you are protected"...
![]() |
Assignee | |
Comment 12•5 years ago
|
||
> > + , mRacyInfo(mozilla::WrapNotNull(new RacyThreadInfo())) > > Could this be a regular field now? Almost. The field comment explains why it's a pointer: > // The thread's RacyThreadInfo. This is an owning pointer. It could be an > // inline member, but we don't do that because RacyThreadInfo is quite large > // (due to the PseudoStack within it), and we have ThreadInfo vectors and so > // we'd end up wasting a lot of space in those vectors for excess elements. > mozilla::NotNull<RacyThreadInfo*> mRacyInfo; When we change PseudoStack from an array to a linked list, then that's a good time to make this an inline member instead of a pointer.
![]() |
Assignee | |
Comment 13•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/af4f0150320bb7139a0a5ec84c8a6df49a5d671a Bug 1359000 (part 1) - Remove unused method PseudoStack::push. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/27ac1ce3ebc8f9a10a84c4c044c50789eaf5333b Bug 1359000 (part 2) - Tweak manual polling of profiler threads. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/572e82ea517b893af4733c2b67a974ed2fa62f12 Bug 1359000 (part 3) - Rename some "stack" variables as "pseudoStack". r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/f93595edab7d54ec4f39308a4d06db85865dd371 Bug 1359000 (part 4) - De-inline profiler_call_{enter,exit}. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/427f09c04b04d5cb1279f05146589539cb7a59f7 Bug 1359000 (part 5) - Introduce TLSInfo. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/c13ffe0d6aaf8c7ad57043f4cb66f65e211dca1e Bug 1359000 (part 6) - Split off RacyThreadInfo from PseudoStack. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/658b857c35f848ad8d9c428c586cce875f9434a3 Bug 1359000 (part 7) - Move mContext and mJSSampling from RacyThreadInfo to ThreadInfo. r=mstange.
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af4f0150320b https://hg.mozilla.org/mozilla-central/rev/27ac1ce3ebc8 https://hg.mozilla.org/mozilla-central/rev/572e82ea517b https://hg.mozilla.org/mozilla-central/rev/f93595edab7d https://hg.mozilla.org/mozilla-central/rev/427f09c04b04 https://hg.mozilla.org/mozilla-central/rev/c13ffe0d6aaf https://hg.mozilla.org/mozilla-central/rev/658b857c35f8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•