Closed Bug 1145187 Opened 9 years ago Closed 9 years ago

Store profiler buffer status per recording

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)

37 Branch
x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(3 files, 9 obsolete files)

10.33 KB, patch
jsantell
: review+
Details | Diff | Splinter Review
7.57 KB, patch
jsantell
: review+
Details | Diff | Splinter Review
43.31 KB, patch
jsantell
: review+
Details | Diff | Splinter Review
This is yet to be decided whether we stop profiling after the buffer is full, or we just have empty data for the start of the profiler where the buffer is overwritten, or maybe something else.
What I think is the best solution is once that buffer is full, keep recording new data as we do now, and the data earlier in the buffer is lost. Chrome does this, but the huge difference there is that they display the buffer's % full during recording, and when it gets to 100%, it just keeps recording, giving more clarity (although maybe we can explain it better) to why the beginning of the profile is missing.
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #1)
> What I think is the best solution is once that buffer is full, keep
> recording new data as we do now, and the data earlier in the buffer is lost.

Makes sense.

> Chrome does this, but the huge difference there is that they display the
> buffer's % full during recording, and when it gets to 100%, it just keeps
> recording.

Do we already have this information?  It would be useful to show.  Then maybe once we are recording past 100% it could just be clear about what is going on in the message: "100% (had to clipped of buffer)".
We do not have the current buffer information. Created bug 1145306 for that.
Depends on: 1145306
The biggest usability problem is that the profiler viewer malfunctions when there is a buffer overflow. If I highlight a particular time range, I seem to get function calls from a different time range. If I then press the + button to zoom in, I definitely zoom in to a different time range. 

This makes overflowed profiler data confusing and practically useless. When I first encountered the issue, I wasted some time trying to make use of the data. Then I discovered everything is fine if I profile for a shorter time, and reported bug 1121649, which is a duplicate of this bug.
Blocks: perf-tool-papercuts
No longer blocks: perf-tool-v2
I've just put a bounty on this bug https://www.bountysource.com/issues/10463218-handle-when-profiler-exceeds-circular-buffer/backers I actually wanted bug #1143224 fixed but it appears that the problem is in this bug.
Summary: Handle when profiler exceeds circular buffer → Display circular buffer status and handle
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attached patch 1145187-backend.patch (obsolete) — Splinter Review
backend patch
Attachment #8598798 - Flags: review?(nfitzgerald)
Comment on attachment 8598798 [details] [diff] [review]
1145187-backend.patch

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

::: toolkit/devtools/server/actors/profiler.js
@@ +59,5 @@
>      return { features: nsIProfilerModule.GetFeatures([]) };
>    },
>  
> +  onGetBufferInfo: function(request) {
> +    let position = {}, totalSize = {}, generation = {};

Maybe re-use objects instead of re-allocate them every time? Doesn't matter too much.

@@ +65,5 @@
> +    return {
> +      position: position.value,
> +      totalSize: totalSize.value,
> +      generation: generation.value
> +    }

semicolon
Attachment #8598798 - Flags: review?(nfitzgerald) → review+
Depends on: 1159480
Priority: -- → P1
Handle when frames are "stretched" due to circular buffer
Attached patch 1145187-poller.patch (obsolete) — Splinter Review
Had this sitting around for some other patch that never was finished. Useful for this patch, and other things in the performance tool.
Attachment #8600667 - Flags: review?(jryans)
Attached patch 1145187-poller.patch (obsolete) — Splinter Review
Made `off()` for poller a promise that resolves upon the completion of any inflight poll calls, a pattern that has caused many intermittents in anything that polls.
Attachment #8600667 - Attachment is obsolete: true
Attachment #8600667 - Flags: review?(jryans)
Attachment #8600713 - Flags: review?(jryans)
Hopefully these smaller patches make the reviews less maddening, Victor
Attachment #8600722 - Flags: review?(vporof)
This is getting large -- making a new bug to handle actually displaying the profiler buffer status.
Summary: Display circular buffer status and handle → Store profiler buffer status per recording
Blocks: 1160900
No longer blocks: perf-tool-papercuts
Fixed front attempting to handle buffer status during actor destruction
Attachment #8600722 - Attachment is obsolete: true
Attachment #8600722 - Flags: review?(vporof)
Attachment #8600728 - Flags: review?(vporof)
Try take 2, this time where I actually destroy the memory actor: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c605043038

Rebased poller patch with a browser.ini change and the facade patch with the memory facades' `yield this._actor.destroy()`, let me know if you want to actually see that diff
Comment on attachment 8600713 [details] [diff] [review]
1145187-poller.patch

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

::: browser/devtools/shared/poller.js
@@ +26,5 @@
> +function Poller (fn, wait, immediate) {
> +  this._fn = fn;
> +  this._wait = wait;
> +  this._immediate = immediate;
> +  this._poll = this._poll.bind(this);

Do you think a |destroy| is needed to clear the bound functions?  Sometimes they can keep an object alive forever, depending on what the object holds a reference to.

::: browser/devtools/shared/test/head.js
@@ +256,5 @@
> +    return Promise.resolve(true);
> +  }
> +  return new Promise(resolve => {
> +    setTimeout(function() {
> +      waitUntil(predicate).then(() => resolve(true));

Recursion may not be the best fit here, since we'll continue to stack another promise on top of the last each interval.  So, once you finally succeed you first have resolve all N promises that were created.

Up to you how bad that seems for a test helper.
Attachment #8600713 - Flags: review?(jryans) → review+
Comment on attachment 8600713 [details] [diff] [review]
1145187-poller.patch

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

::: browser/devtools/shared/poller.js
@@ +26,5 @@
> +function Poller (fn, wait, immediate) {
> +  this._fn = fn;
> +  this._wait = wait;
> +  this._immediate = immediate;
> +  this._poll = this._poll.bind(this);

For my cases, I don't think that'll come up -- as everything referencing the poller and the bound function should no longer be accessible, and should be swept via CC, but I don't think it can hurt to add a destroy() function that turns off polling, as well as unbinds the function. Will add that!

::: browser/devtools/shared/test/head.js
@@ +256,5 @@
> +    return Promise.resolve(true);
> +  }
> +  return new Promise(resolve => {
> +    setTimeout(function() {
> +      waitUntil(predicate).then(() => resolve(true));

It doesn't seem great, but this was copied over from another test/head.js and it hasn't had problems yet so it's probably alright for this case (unless we have a large stack of promises)
Rebased, added destroy method and tests for poller
Attachment #8600713 - Attachment is obsolete: true
Attachment #8601034 - Flags: review+
Rebased server side component, added another trait to root actor for getBufferInfo ability since profiler does not use ActorClass
Attachment #8598798 - Attachment is obsolete: true
Attachment #8601035 - Flags: review+
Other patches look good, will land once fx-team up; re-trying for the buffer facade patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aaaad06d08a
Attachment #8600728 - Attachment is obsolete: true
Attachment #8600728 - Flags: review?(vporof)
Attachment #8601053 - Flags: review?(vporof)
https://hg.mozilla.org/mozilla-central/rev/80bb01392379
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Reopening, still awaiting review and patch landing for Attachment #8601053 [details] [diff]
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Comment on attachment 8601053 [details] [diff] [review]
1145187-bufferstatus-facade.patch

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

I have questions, but otherwise r+ with comments.

::: browser/devtools/performance/modules/actors.js
@@ +125,5 @@
>      };
>  
>      yield this.startProfiler(profilerOptions);
> +    // Get the buffer status at the starting point
> +    let bufferStatus = yield this.getBufferInfo();

Why not make `isActive` return the buffer info, instead of having it as a separate method? This way, you don't even really need a trait, do you?

Also, might be a good idea to make `startProfiler` also return the buffer status.

::: browser/devtools/performance/modules/front.js
@@ +375,5 @@
>      // the underlying memory and timeline actors. If we're still recording,
>      // juse use Date.now() for the memory and timeline end times, as those
>      // are only used in tests.
>      if (!this.isRecording()) {
> +      yield this._profiler.stop();

The profiler is stopped anyway when the last connection closes, right? I don't understand why this is here.

::: browser/devtools/performance/modules/recording-model.js
@@ +90,5 @@
>    /**
>     * Sets up the instance with data from the SharedPerformanceConnection when
>     * starting a recording. Should only be called by SharedPerformanceConnection.
>     */
> +  _populate: function (info) {

Why were these methods made private, since they're being called from outside this file?
Attachment #8601053 - Flags: review?(vporof)
Comment on attachment 8601053 [details] [diff] [review]
1145187-bufferstatus-facade.patch

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

::: browser/devtools/performance/modules/actors.js
@@ +125,5 @@
>      };
>  
>      yield this.startProfiler(profilerOptions);
> +    // Get the buffer status at the starting point
> +    let bufferStatus = yield this.getBufferInfo();

that is not a bad idea

::: browser/devtools/performance/modules/front.js
@@ +375,5 @@
>      // the underlying memory and timeline actors. If we're still recording,
>      // juse use Date.now() for the memory and timeline end times, as those
>      // are only used in tests.
>      if (!this.isRecording()) {
> +      yield this._profiler.stop();

This stops the polling for buffer status, not the profiler itself.

::: browser/devtools/performance/modules/recording-model.js
@@ +90,5 @@
>    /**
>     * Sets up the instance with data from the SharedPerformanceConnection when
>     * starting a recording. Should only be called by SharedPerformanceConnection.
>     */
> +  _populate: function (info) {

RecordingModel functions almost as if a fake Actor itself, like the PerformanceFront. Like we have a main actor in each tool that can return children actors, that's how I look at the recording models. There is only one case where this is not the case (importing a recording, I believe), where the model is created outside of the PerformanceFront.

tldr: Only the PerformanceFront should be able to access these private methods.
Made those changes. Still keeping the buffer method on profiler -- but using the isActive method which will be more resilient for backwards compat.
Attachment #8601053 - Attachment is obsolete: true
Attachment #8601845 - Flags: review?(vporof)
Attachment #8601845 - Flags: review?(vporof) → review+
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #29)
> Comment on attachment 8601053 [details] [diff] [review]
> 
> This stops the polling for buffer status, not the profiler itself.
> 

Please add a comment explaining this.
updated with comment
Attachment #8601845 - Attachment is obsolete: true
Attachment #8601868 - Flags: review+
Fix xpcshell test, also fixed issue with getMappedSelection called before overview graphs are rendered, which is probably the cause of bug 1160828

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b186200747
Attachment #8601868 - Attachment is obsolete: true
Attachment #8601898 - Flags: review+
Fixing a test case to not fail due to race conditions and we're good
Attachment #8601898 - Attachment is obsolete: true
Attachment #8602133 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e6e4fed5ecbf
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Depends on: 1162385
Thanks Andrei!

Donating bounty+ to EB research[0] in memory of Mihai[1].

[0] https://donate.ebresearch.org/
[1] http://incompleteness.me/blog/2015/02/09/console-dot-mihai/
The second bounty I pay to a FF developer, the second one that goes to EB research :P I'm starting to feel sand that I never met Mihai. I am Romanian and we had some mutual friends but I never got to know him.

I'm glad that you decided to send the money to a noble cause, even if it is a small amount. I hope more FF users start to support FF development and Mozilla in genera.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.