750989 - Pause Profiler during saving

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Currently we don't pause the profiler while saving. Pausing the profiler eliminates some race condition when the profile is being saved and written to at the same time and prevent profile saving samples from being recorded in the profile.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Increased sampling frequency to 1 microsecond on mac. The empty profile bug occured very frequently. With this patch applied it no longer appears as well as profile saving samples were no longer recorded.

Comment 2

[Markus Stange [:mstange]]

The description you posted sounds like it belongs to a different patch.

I think a better solution for this bug would be remember the current write position before reading the samples, and then only read up to this position. Then you could still profile GetProfile with the built-in profiler (by getting another profile afterwards) without disabling this patch.

Comment 3

[Benoit Girard (:BenWa)]

The description was meant to explain how I tested the patch.

There's current a race condition that can cause profile saving to fail if you grab a sample while saving. I feel like this might be a regression with some of the jank changes done to the circular buffer code but I'm not sure.

In any case I feel like sampling profile saving could go either way. Either we record it or we don't. I would rather not record it since it makes the code simpler. If we care about profile saving we can use an external profiler to catch that.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 620165 [details] [diff] [review]
patch

Also asking review from mstange since you seem to disagree with this patch.

Comment 5

[Mozilla RelEng Bot]

Try run for 62fdf76fe945 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=62fdf76fe945
Results (out of 107 total builds):
    success: 79
    warnings: 28
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-62fdf76fe945

Comment 6

[Mozilla RelEng Bot]

Try run for 6b0d8d1b3629 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6b0d8d1b3629
Results (out of 113 total builds):
    success: 85
    warnings: 26
    other: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6b0d8d1b3629

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 620165 [details] [diff] [review]
patch

(In reply to Benoit Girard (:BenWa) from comment #3)
> The description was meant to explain how I tested the patch.

Ah, I see. I didn't understand how the empty profile bug relates to pausing the profiler, but now I do.

> There's current a race condition that can cause profile saving to fail if
> you grab a sample while saving.

I see how that can happen as soon as we sample faster than we read. Or rather, there's no way it doesn't happen if that's the case.

Say w and f are 3 and we have already wrapped at least once. Then r is 4. Now we start reading at 4, but while we're stringifying the first sample, two entries are added and flushed, so w and f are now 5. After the stringification of the first entry, our local readPos is incremented to 5, compared to f, and we stop. Then our resulting profile has only one entry.
Even if we cached f locally before starting to read, we'd be reading data that's already overwritten with newer data from the next round, and we don't want that.

I now agree with the approach this patch takes. The only clean way of doing what I proposed is to copy the whole buffer before reading from it, but even then we'd have to pause the profiler during the copy if we want it to be consistent. But that would be too much effort.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 620165 [details] [diff] [review]
patch

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

This seems to only fix OS X, and should include an explanation of why it's needed.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch v2

Fixed

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch v3

fixed a few typos

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch v4

Fixed built error:
https://tbpl.mozilla.org/?tree=Try&rev=09280d908118

Comment 12

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdea12fb0063

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdea12fb0063