Merge ThreadInfo and ThreadProfile

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

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

Comment 1

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

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
|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

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

Comment 4

2 years ago
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

2 years ago
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)
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+
You need to log in before you can comment on or make changes to this bug.