Closed
Bug 1437428
Opened 7 years ago
Closed 7 years ago
Rework ThreadInfo some more
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: mstange, Assigned: mstange)
References
Details
Attachments
(2 files)
ThreadInfo currently contains a bunch of information about a thread, and different types of information are used in different settings.
Some information is used only while the thread is alive and registered, regardless of whether the thread is being profiled:
[Group A]
- RacyInfo + PseudoStack
- mPlatformData
- mThread (the nsIEventTarget)
- mContext (the JSContext*)
- mJSSampling
Some information is used only for threads that are currently being profiled:
[Group B]
- mResponsiveness
- mLastSample
Some information is used only for threads that are currently being profiled or which have been profiled before and have shut down during profiling but still have data in the profile buffer:
[Group C]
- mPartialProfile
Some information is only used for threads which have already shut down, but which have been profiled before and still have data in the buffer (and the profiler is still running):
[Group D]
- mUnregisterTime
- mBufferPositionWhenUnregistered
Some information is useful for any thread that has been known to the profiler at some point and for which any of the other information is still in use:
[Group E]
- mRegisterTime
- mName
- mThreadId
- mIsMainThread
I'd like to split up ThreadInfo accordingly. Most importantly, I'd like to make sure that any buffer positions (mLastSample and mBufferPositionWhenUnregistered) are only stored on objects that will get destroyed when the profiler is stopped, which is when the buffer is destroyed and any buffer positions become invalid.
However, 5 groups is a bit much. I think groups B, C and D can be grouped into the same object.
Here are the names I've come up with:
- RegisteredThread + RacyRegisteredThread, contains fields from group A
- ProfiledThreadData, contains fields from groups B, C and D
- ThreadInfo, contains fields from group E
ThreadInfo will become an immutable, threadsafe-refcounted object that is owned both by RegisteredThread and by ProfiledThreadData.
RegisteredThreads will be owned by CorePS, and ProfiledThreadData objects will be owned by ActivePS.
The patches in this bug will also fix bug 1434440.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Blocks: profiler-cleanup
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8950137 [details]
Bug 1437428 - Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData.
https://reviewboard.mozilla.org/r/219402/#review225118
Code analysis found 3 defects in this patch:
- 3 defects found by clang-tidy
You can run this analysis locally with:
- `./mach static-analysis check path/to/file.cpp` (C/C++)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: tools/profiler/core/platform.cpp:1381
(Diff revision 1)
> - const Registers& aRegs, ProfileBuffer& aBuffer)
> + const TimeStamp& aNow, const Registers& aRegs,
> + ProfileBuffer& aBuffer)
> {
> // WARNING: this function runs within the profiler's "critical section".
>
> - DoSharedSample(aLock, /* isSynchronous = */ true, aThreadInfo, aNow, aRegs,
> + DoSharedSample(aLock, /* isSynchronous = */ true, aRegisteredThread, aNow,
Warning: Argument name 'issynchronous' in comment does not match parameter name 'aissynchronous' [clang-tidy: misc-argument-comment]
DoSharedSample(aLock, /* isSynchronous = */ true, aRegisteredThread, aNow,
^
/* aIsSynchronous = */
::: tools/profiler/core/platform.cpp:1382
(Diff revision 1)
> + ProfileBuffer& aBuffer)
> {
> // WARNING: this function runs within the profiler's "critical section".
>
> - DoSharedSample(aLock, /* isSynchronous = */ true, aThreadInfo, aNow, aRegs,
> - /* lastSample = */ nullptr, aBuffer);
> + DoSharedSample(aLock, /* isSynchronous = */ true, aRegisteredThread, aNow,
> + aRegs, /* lastSample = */ nullptr, aBuffer);
Warning: Argument name 'lastsample' in comment does not match parameter name 'alastsample' [clang-tidy: misc-argument-comment]
aRegs, /* lastSample = */ nullptr, aBuffer);
^
/* aLastSample = */
::: tools/profiler/core/platform.cpp:1396
(Diff revision 1)
> {
> // WARNING: this function runs within the profiler's "critical section".
>
> ProfileBuffer& buffer = ActivePS::Buffer(aLock);
>
> - DoSharedSample(aLock, /* isSynchronous = */ false, aThreadInfo, aNow, aRegs,
> + DoSharedSample(aLock, /* isSynchronous = */ false, aRegisteredThread, aNow,
Warning: Argument name 'issynchronous' in comment does not match parameter name 'aissynchronous' [clang-tidy: misc-argument-comment]
DoSharedSample(aLock, /* isSynchronous = */ false, aRegisteredThread, aNow,
^
/* aIsSynchronous = */
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8950136 [details]
Bug 1437428 - Make PseudoStack a member of RacyInfo instead of inheriting from it.
https://reviewboard.mozilla.org/r/219400/#review225440
Nice change. And smaller than I would have expected!
Attachment #8950136 -
Flags: review?(n.nethercote) → review+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8950137 [details]
Bug 1437428 - Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData.
https://reviewboard.mozilla.org/r/219402/#review225444
::: tools/profiler/core/ProfiledThreadData.h:42
(Diff revision 1)
> -//
> -// Note: A thread's ThreadInfo can be held onto after the thread itself exits,
> -// because we may need to output profiling information about that thread. But
> -// some of the fields in this class are only relevant while the thread is
> -// alive. It's possible that this class could be refactored so there is a
> -// clearer split between those fields and the fields that are still relevant
> +// the profiler is running, for any threads (both alive and dead) whose thread
> +// name matches the "thread filter" in the current profiler run.
> +// ProfiledThreadData objects may be kept alive even after the thread is
> +// unregistered, as long as there is still data for that thread in the profiler
> +// buffer.
> +// Accesses to this class are protected by the profiler state lock.
This is a multi-paragraph comment. I would put blank lines between each paragraph :)
::: tools/profiler/core/ProfiledThreadData.h:46
(Diff revision 1)
> -// alive. It's possible that this class could be refactored so there is a
> -// clearer split between those fields and the fields that are still relevant
> -// after the thread exists.
> -class ThreadInfo final
> +// buffer.
> +// Accesses to this class are protected by the profiler state lock.
> +// Created as soon as the following are true for the thread:
> +// - The profiler is running
> +// - The thread matches the profiler's thread filter
> +// - The thread is registered with the profiler.
I would put ", and" at the end of the first and second bullet points, to make it clearer it's a conjunction.
Attachment #8950137 -
Flags: review?(n.nethercote) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8950137 [details]
Bug 1437428 - Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData.
https://reviewboard.mozilla.org/r/219402/#review225450
Good change. I tried splitting ThreadInfo into LiveThreadInfo and DeadThreadInfo at one point, but it didn't work out nicely. This patch goes quite a bit further in that direction.
::: tools/profiler/core/ProfiledThreadData.h:96
(Diff revision 1)
>
> private:
> - bool mIsBeingProfiled;
> + const RefPtr<ThreadInfo> mThreadInfo;
> +
> + mozilla::Maybe<uint64_t> mBufferPositionWhenUnregistered;
> + mozilla::TimeStamp mUnregisterTime;
The B/C/D categorisation in comment 0 is helpful. I think it would worth grouping these fields accordingly and putting comments above them explaining when they are used.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/0fcad4eaabb6
Make PseudoStack a member of RacyInfo instead of inheriting from it. r=njn
https://hg.mozilla.org/integration/autoland/rev/b915e160a690
Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData. r=njn
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0fcad4eaabb6
https://hg.mozilla.org/mozilla-central/rev/b915e160a690
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 12•7 years ago
|
||
Backed out 2 changesets (bug 1437428) for frequent xpcshell failures on marAppApplyUpdateStageOldVersionFailure.js
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162907613&repo=autoland&lineNumber=2434
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b915e160a690eb75d647c3681682064f87869f10&filter-searchStr=linux%20debug%20xpcshell
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad133cd410a719c0b67e61b8d3b1c77a32fd80a9
Status: RESOLVED → REOPENED
Flags: needinfo?(mstange)
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Comment 13•7 years ago
|
||
(In reply to Cosmin Sabou [:cosmin_s] from comment #12)
> Backed out 2 changesets (bug 1437428) for frequent xpcshell failures on
> marAppApplyUpdateStageOldVersionFailure.js
That was bug 1439118.
Assignee | ||
Comment 14•7 years ago
|
||
I've attached a fix for the crash in bug 1440783. Once that bug lands, this is can re-land.
Depends on: 1440783
Flags: needinfo?(mstange)
Comment 15•7 years ago
|
||
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/d2a379737c4b
Make PseudoStack a member of RacyInfo instead of inheriting from it. r=njn
https://hg.mozilla.org/integration/autoland/rev/ef43be3bcc75
Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData. r=njn
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d2a379737c4b
https://hg.mozilla.org/mozilla-central/rev/ef43be3bcc75
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•