Last Comment Bug 750989 - Pause Profiler during saving
: Pause Profiler during saving
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Benoit Girard (:BenWa)
:
:
Mentors:
Depends on: 764616
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-01 18:52 PDT by Benoit Girard (:BenWa)
Modified: 2012-06-13 15:16 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.62 KB, patch)
2012-05-01 18:54 PDT, Benoit Girard (:BenWa)
jmuizelaar: review-
mstange: review+
Details | Diff | Splinter Review
patch v2 (7.49 KB, patch)
2012-05-17 19:30 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
patch v3 (7.71 KB, patch)
2012-05-18 14:02 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
patch v4 (7.73 KB, patch)
2012-05-21 13:43 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-05-01 18:52:32 PDT
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) 2012-05-01 18:54:22 PDT
Created attachment 620165 [details] [diff] [review]
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] 2012-05-02 04:53:24 PDT
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) 2012-05-02 06:37:44 PDT
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) 2012-05-02 06:39:14 PDT
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 2012-05-03 05:00:24 PDT
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/b56girard@gmail.com-62fdf76fe945
Comment 6 Mozilla RelEng Bot 2012-05-03 06:15:20 PDT
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/b56girard@gmail.com-6b0d8d1b3629
Comment 7 Markus Stange [:mstange] 2012-05-07 01:47:43 PDT
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] 2012-05-09 15:15:28 PDT
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) 2012-05-17 19:30:50 PDT
Created attachment 624995 [details] [diff] [review]
patch v2

Fixed
Comment 10 Benoit Girard (:BenWa) 2012-05-18 14:02:16 PDT
Created attachment 625252 [details] [diff] [review]
patch v3

fixed a few typos
Comment 11 Benoit Girard (:BenWa) 2012-05-21 13:43:22 PDT
Created attachment 625749 [details] [diff] [review]
patch v4

Fixed built error:
https://tbpl.mozilla.org/?tree=Try&rev=09280d908118
Comment 13 Ed Morley [:emorley] 2012-05-22 06:32:43 PDT
https://hg.mozilla.org/mozilla-central/rev/fdea12fb0063

Note You need to log in before you can comment on or make changes to this bug.