Simplify sleep handling in PseudoStack

RESOLVED FIXED in Firefox 54

Status

()

Core
Gecko Profiler
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 months ago
It's much more complex than it needs to be.
(Assignee)

Comment 1

6 months ago
Created attachment 8836564 [details] [diff] [review]
(part 1) - Reformat PseudoStack.h

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

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

Comment 2

6 months ago
Created attachment 8836565 [details] [diff] [review]
(part 2) - Remove out-of-date comment

Bug 1337189 part 17 was the change that made this comment no longer true.
Attachment #8836565 - Flags: review?(mstange)
(Assignee)

Comment 3

6 months ago
Created attachment 8836566 [details] [diff] [review]
(part 3) - Simplify sleep handling in PseudoStack

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

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

6 months ago
Created attachment 8836591 [details] [diff] [review]
(part 3) - Simplify sleep handling in PseudoStack

Fix a couple of minor things.
Attachment #8836591 - Flags: review?(mstange)
(Assignee)

Updated

6 months ago
Attachment #8836566 - Attachment is obsolete: true
Attachment #8836566 - Flags: review?(mstange)
(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
Attachment #8836564 - Flags: review?(mstange) → review+
Attachment #8836565 - Flags: review?(mstange) → review+
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+
(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

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

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

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