Improve the profiler's Thread class

RESOLVED FIXED in Firefox 54

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 months ago
It's a clumsy cross-platform abstraction. GetCurrentId() and tid_t are the only
genuinely cross-platform things in it, and the patch keeps those in place.

The remaining methods are not implemented on all platforms (none are
implemented on Linux) and the fields are all used on either Mac or Windows, but
not both. So the patch introduces MacOSThread and Win32Thread classes and moves
the relevant Thread method implementations and fields into them. There is
sufficiently little overlap between the two new classes that there is no point
sharing a base class between them.

The patch also changes some of the existing code to use Gecko style, e.g.
|mFoo| instead of |foo_| for class fields.
(Assignee)

Comment 1

5 months ago
Created attachment 8830125 [details] [diff] [review]
Improve the profiler's Thread class
Attachment #8830125 - Flags: review?(mstange)
Comment on attachment 8830125 [details] [diff] [review]
Improve the profiler's Thread class

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

Ok. Without the standalone build requirement, there's not much reason not to use PR_CreateThread and friends, is there?
Attachment #8830125 - Flags: review?(mstange) → review+
(Assignee)

Comment 3

5 months ago
Created attachment 8831001 [details] [diff] [review]
Improve the profiler's Thread class

Sorry that I'm asking for re-review, but I realized that Win32Thread and
MacOSThread both had a single sub-class (two different versions of
SamplerThread) and that this sub-classing is unnecessary. Removing it gets rid
of some virtual methods and simplifies thread naming on Mac.
Attachment #8831001 - Flags: review?(mstange)
(Assignee)

Updated

5 months ago
Attachment #8830125 - Attachment is obsolete: true
Attachment #8831001 - Flags: review?(mstange) → review+
(Assignee)

Comment 4

5 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/31af743f4a0273ad198f50fe8d20dd6978027979
Bug 1333655 - Improve the profiler's Thread class. r=mstange.

Comment 5

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/31af743f4a02
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.