Closed
Bug 1338957
Opened 8 years ago
Closed 8 years ago
Simplify sleep handling in PseudoStack
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
It's much more complex than it needs to be.
Assignee | ||
Comment 1•8 years ago
|
||
This patch:
- Reformats PseudoStack.h to more closely follow Mozilla style.
- Rewrites some comments to make them more readable, e.g. by properly
delimiting sentences with upper-case letters and full stops.
- Replaces sMin() with std::min(), because <algorithm> no longer causes
problems.
- Reorders PseudoStack so that all the data members are at the end, which makes
them easier to see.
Attachment #8836564 -
Flags: review?(mstange)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Bug 1337189 part 17 was the change that made this comment no longer true.
Attachment #8836565 -
Flags: review?(mstange)
Assignee | ||
Comment 3•8 years ago
|
||
Sleep handling is currently done with two counters (mSleepId, mSleepIdObserved)
and a boolean (mSleeping). This patch replaces it with a single tri-state
variable (mSleep), which is enough to replicate the functionality.
The patch also clearly documents how this variable is accessed from multiple
threads and the potential inaccuracies that can result from races.
Attachment #8836566 -
Flags: review?(mstange)
Assignee | ||
Comment 4•8 years ago
|
||
mstange, I'm interested to hear if you think this (from part 3) is worth pursuing:
// This latter inaccuracy could be avoided by moving the
// CanDuplicateLastSampleDueToSleep() check within the thread-freezing code,
// e.g. the section where Tick() is called. But that would reduce the
// effectiveness of the optimization because more code would have to be run
// before we can tell that duplication is allowed.
In case it helps, bug 963158 is the bug that introduced the sleep handling code, and bug 963158 comment 3 has some performance numbers, which I have not yet tried to replicate.
Assignee | ||
Comment 5•8 years ago
|
||
Fix a couple of minor things.
Attachment #8836591 -
Flags: review?(mstange)
Assignee | ||
Updated•8 years ago
|
Attachment #8836566 -
Attachment is obsolete: true
Attachment #8836566 -
Flags: review?(mstange)
Comment 6•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Created attachment 8836591 [details] [diff] [review]
> (part 3) - Simplify sleep handling in PseudoStack
nano-comment:
+ // issues. (In actual fast, this case is impossible. In order to go from
s/fast/fact
Updated•8 years ago
|
Attachment #8836564 -
Flags: review?(mstange) → review+
Updated•8 years ago
|
Attachment #8836565 -
Flags: review?(mstange) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8836591 [details] [diff] [review]
(part 3) - Simplify sleep handling in PseudoStack
Review of attachment 8836591 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good! I accidentally reviewed the older version of this patch first, so I'm happy that you fixed the "minor" mistake of calling setAwake instead of setSleeping :)
Attachment #8836591 -
Flags: review?(mstange) → review+
Comment 8•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #4)
> mstange, I'm interested to hear if you think this (from part 3) is worth
> pursuing:
I do not think this is worth pursuing. Sampling-based profilers have some inherent inaccuracy; this one is not a source of inaccuracy that's worth worrying about, in my opinion.
In fact, it's arguable whether this is even an inaccuracy at all. You could consider sampling to have two phases: (1) measuring the current state of the thread's stack, and (2) writing out the collected information to the buffer. Phase (2) could be delayed arbitrarily and we wouldn't care - as long as what we write out to the buffer reflects the truth at the time of measuring. In the sleeping case, the "measuring phase" is the time at which we call CanDuplicateLastSampleDueToSleep(), and it returns the truth as observed at that time.
Assignee | ||
Comment 9•8 years ago
|
||
> Looks good! I accidentally reviewed the older version of this patch first,
> so I'm happy that you fixed the "minor" mistake of calling setAwake instead
> of setSleeping :)
A try push showed that one quickly. Hooray for assertions!
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/292709890f3870bca9e7855e3ad37ea21acbffe1
Bug 1338957 (part 1) - Reformat PseudoStack.h. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/da489be2ba589598ae21f0c20cd986b27cbd81c3
Bug 1338957 (part 2) - Remove out-of-date comment. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b2093bb0be6d264cbb223674d1dee23692254e7
Bug 1338957 (part 3) - Simplify sleep handling in PseudoStack. r=mstange.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/292709890f38
https://hg.mozilla.org/mozilla-central/rev/da489be2ba58
https://hg.mozilla.org/mozilla-central/rev/9b2093bb0be6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•