Rework ThreadInfo some more

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

Last year
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

Last year

Comment 3

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/0fcad4eaabb6
https://hg.mozilla.org/mozilla-central/rev/b915e160a690
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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 → ---
(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

Updated

Last year
Blocks: 1437679
Assignee

Updated

Last year
Blocks: 1436987
Assignee

Comment 14

Last year
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

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2a379737c4b
https://hg.mozilla.org/mozilla-central/rev/ef43be3bcc75
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.