1335595 - Merge ThreadInfo and ThreadProfile

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicholas Nethercote [inactive]]

ThreadInfo and ThreadProfile are so incestuous that merging them makes things clearer. And I have some minor related cleanups.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove the profiler's ASSERT macro

MOZ_ASSERT is the standard macro.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove useless null checks in Sampler::InplaceTick()

|sample| is non-null at all callsites. The function derefs |sampler| before
doing any of the null checks, anyway.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove ThreadProfile::AsSyncProfile()

It's unused.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Merge ThreadProfile into ThreadInfo

ThreadInfo and ThreadProfile are hopelessly intertwined.

- ThreadInfo has an owning pointer to ThreadProfile. ThreadProfile has a raw
  back pointer to ThreadInfo. A reference to one is as good as a reference to
  the other, and references to one frequently reach into the other.

- An exception is SyncProfile, a sub-class of ThreadProfile, which instead has
  an owning pointer to its ThreadInfo. (This makes the SyncProfile's destructor
  dubious, because it deletes the ThreadInfo, which could conceivably re-call
  into SyncProfile's destructor.)

- ThreadProfile also has copies of five ThreadInfo fields (mThreadId,
  mIsMainThread, mPlatformData, mStackTop, mPseudoStack) even though it also
  has direct ThreadInfo access via the back pointer.

The only good reason for having the two classes separate is that some
ThreadInfo objects have a null ThreadProfile pointer. But this doesn't justify
the entanglement.

So this patch merges ThreadProfile into ThreadInfo. It visually separates the
methods and fields related to profiles to partially preserve the original
meaning of the split. The new ThreadInfo::hasProfile() method replaces
ThreadInfo::Profile() as the indicator of whether a ThreadInfo has associated
profile data.

Notable points of simplification:

- The five duplicated fields are no longer duplicated.

- NewSyncProfile(), RegisterThread() no longer create ThreadProfile objects.

- ~SyncProfile() becomes trivial.

- ThreadInfo::SetPendingDelete() is simpler.

- Overall it removes ~80 lines of code.

Much of the rest is just plumbing changes.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove ThreadResponsiveness::mThreadInfo

ThreadInfo contains a ThreadResponsiveness, and then ThreadResponsiveness has a
pointer back to its containing ThreadInfo, which is gross.

The back pointer is only needed for Update(), and it's easy to pass in the
necessary info instead via a new method UpdateThreadResponsiveness().

This change also means ThreadInfo::GetThread() can be removed.

Comment 6

[Nicholas Nethercote [inactive]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1f9e3a709bcefcb12d4d47ece9a78998684fc09

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8832328 [details] [diff] [review]
(part 2) - Remove useless null checks in Sampler::InplaceTick()

Review of attachment 8832328 [details] [diff] [review]:
-----------------------------------------------------------------

This suggests that it should become a reference instead of a pointer.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a81f3f6cf15b4d32437281df3a68899f175eb6
Bug 1335595 (part 1) - Remove the profiler's ASSERT macro. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5aac5826058a4dbfb8c9a0a5a2070870eca83874
Bug 1335595 (part 2) - Remove useless null checks in Sampler::InplaceTick(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2ec36e2bee2d63fecaec9dab177d79a0b39332
Bug 1335595 (part 3) - Remove ThreadProfile::AsSyncProfile(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82e687d1494c8eca73dd7464d634cac24429f1aa
Bug 1335595 (part 4) - Merge ThreadProfile into ThreadInfo. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2aa01fb8954a756814bb9bf8b31b3096cf1302
Bug 1335595 (part 5) - Remove ThreadResponsiveness::mThreadInfo. r=mstange.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3a81f3f6cf1
https://hg.mozilla.org/mozilla-central/rev/5aac5826058a
https://hg.mozilla.org/mozilla-central/rev/9a2ec36e2bee
https://hg.mozilla.org/mozilla-central/rev/82e687d1494c
https://hg.mozilla.org/mozilla-central/rev/ee2aa01fb895