Merge ThreadInfo and ThreadProfile

RESOLVED FIXED in Firefox 54

Status

()

Core
Gecko Profiler
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

7 months ago
ThreadInfo and ThreadProfile are so incestuous that merging them makes things clearer. And I have some minor related cleanups.
(Assignee)

Comment 1

7 months ago
Created attachment 8832327 [details] [diff] [review]
(part 1) - Remove the profiler's ASSERT macro

MOZ_ASSERT is the standard macro.
Attachment #8832327 - Flags: review?(mstange)
(Assignee)

Updated

7 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 months ago
Created attachment 8832328 [details] [diff] [review]
(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.
Attachment #8832328 - Flags: review?(mstange)
(Assignee)

Comment 3

7 months ago
Created attachment 8832329 [details] [diff] [review]
(part 3) - Remove ThreadProfile::AsSyncProfile()

It's unused.
Attachment #8832329 - Flags: review?(mstange)
(Assignee)

Comment 4

7 months ago
Created attachment 8832330 [details] [diff] [review]
(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.
Attachment #8832330 - Flags: review?(mstange)
(Assignee)

Comment 5

7 months ago
Created attachment 8832331 [details] [diff] [review]
(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.
Attachment #8832331 - Flags: review?(mstange)
(Assignee)

Comment 6

7 months ago
Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1f9e3a709bcefcb12d4d47ece9a78998684fc09
Attachment #8832327 - Flags: review?(mstange) → review+
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.
Attachment #8832328 - Flags: review?(mstange) → review+
Attachment #8832329 - Flags: review?(mstange) → review+
Attachment #8832330 - Flags: review?(mstange) → review+
Attachment #8832331 - Flags: review?(mstange) → review+
(Assignee)

Comment 8

7 months ago
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

7 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.