1338957 - Simplify sleep handling in PseudoStack

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicholas Nethercote [inactive]]

It's much more complex than it needs to be.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove out-of-date comment

Bug 1337189 part 17 was the change that made this comment no longer true.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Simplify sleep handling in PseudoStack

Fix a couple of minor things.

Comment 6

[Julian Seward [:jseward]]

(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

Comment 7

[Markus Stange [:mstange]]

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 :)

Comment 8

[Markus Stange [:mstange]]

(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.

Comment 9

[Nicholas Nethercote [inactive]]

> 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!

Comment 10

[Nicholas Nethercote [inactive]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/292709890f38
https://hg.mozilla.org/mozilla-central/rev/da489be2ba58
https://hg.mozilla.org/mozilla-central/rev/9b2093bb0be6