1437428 - Rework ThreadInfo some more

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

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 1

[Markus Stange [:mstange]]

Attachment: Bug 1437428 - Make PseudoStack a member of RacyInfo instead of inheriting from it.

Review commit: https://reviewboard.mozilla.org/r/219400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219400/

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1437428 - Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData.

The file copies and renames make this patch look a bit confusing. Here's what
happens:

ThreadInfo.h:
Most of the code gets moved into RegisteredThread.h and ProfiledThreadData.h,
but a small piece remains in ThreadInfo.h.

ThreadInfo.cpp:
Gets split into RegisteredThread.cpp and ProfiledThreadData.cpp.
ThreadInfo.cpp itself goes away.

In the mercurial changeset, I've marked ThreadInfo.h as being copied to both
RegisteredThread.h and to ProfiledThreadData.h, and ThreadInfo.cpp as being
copied to RegisteredThread.cpp and as being renamed to ProfiledThreadData.cpp.

Review commit: https://reviewboard.mozilla.org/r/219402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219402/

Comment 3

[Code Review Bot [:reviewbot]]

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

[Nicholas Nethercote [inactive]]

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!

Comment 5

[Nicholas Nethercote [inactive]]

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.

Comment 6

[Nicholas Nethercote [inactive]]

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 7

[Markus Stange [:mstange]]

Comment on attachment 8950136 [details]
Bug 1437428 - Make PseudoStack a member of RacyInfo instead of inheriting from it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219400/diff/1-2/

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8950137 [details]
Bug 1437428 - Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219402/diff/1-2/

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8950137 [details]
Bug 1437428 - Split ThreadInfo into three classes: ThreadInfo, RegisteredThread and ProfiledThreadData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219402/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0fcad4eaabb6
https://hg.mozilla.org/mozilla-central/rev/b915e160a690

Comment 12

[Cosmin Sabou [:CosminS]]

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

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(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.

Comment 14

[Markus Stange [:mstange]]

I've attached a fix for the crash in bug 1440783. Once that bug lands, this is can re-land.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d2a379737c4b
https://hg.mozilla.org/mozilla-central/rev/ef43be3bcc75