Closed Bug 1150863 Opened 10 years ago Closed 10 years ago

about:performance should be on pause by default

Categories

(Toolkit :: Performance Monitoring, defect)

Product:
Toolkit
Component:
Performance Monitoring
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: Yoric, Assigned: wrahman0)

Details

Attachments

(1 file, 4 obsolete files)

bug1150863_add_play_pause_button.diff
4.73 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
At the moment, about:performance is pretty useless. One of the reasons is that it changes so fast that it's quite hard to copy&paste or use it to request assistance. We should change its behavior as follows: - by default, display a snapshot of an interval of ~1 second sampled while opening the page; - a "play/pause" button lets users decide when they want a live view.
What would this feature require? Can I have an estimation of the difficulty of this bug?
Flags: needinfo?(dteller)
Well, this would require changing the source code of https://dxr.mozilla.org/mozilla-central/source/toolkit/components/aboutperformance/content/aboutPerformance.js and https://dxr.mozilla.org/mozilla-central/source/toolkit/components/aboutperformance/content/aboutPerformance.xhtml. We would need to add a button to the .xhtml, and make it control the `setInterval` time the .js. All in all, this should be pretty easy for someone comfortable with setTimeout and setInterval.
Flags: needinfo?(dteller)
Awesome, I would like to give this a shot. I will let you know about my progress.
David, you asked only for a "play/pause" button. I think it would be even more useful if the tool would offer multiple update interval options, like Sysinternals Process Explorer does: ------------- * 0.5 seconds * 1 second * 2 seconds * 5 seconds * 10 seconds ------------- * Paused ------------- Agree?
Flags: needinfo?(dteller)
Jason: Well, I was planning to add this later, but yes, that's a good idea.
Flags: needinfo?(dteller)
@David: Just an update: I have been fiddling around with the play/pause button functionality. Its coming along quite well. I think this patch is a good place to implement Jason's interval suggestion as well. What do you think?
Flags: needinfo?(dteller)
This is a WIP. Just showing you my progress. Please let me know what you think.
Attachment #8592603 - Flags: review?(dteller)
Also, can I get assigned to this bug?
Assignee: nobody → wrahman0
Flags: needinfo?(dteller)
If implemnenting Jason's interval suggestion is simple enough, feel free to do it in this patch.
Comment on attachment 8592603 [details] [diff] [review] bug1150863_add_play_pause_button.diff Review of attachment 8592603 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/aboutperformance/content/aboutPerformance.js @@ +161,5 @@ > + if (timerId != null){ > + clearInterval(timerId); > + timerId = null; > + } > +} Could you group `timerId`, `startUpdate` and `stopUpdate` in a single object? Say `AutoUpdate`, with a field `_timerId` and methods `pause` and `unpause`? @@ +167,5 @@ > function go() { > // Compute initial state immediately, then wait a little > // before we start computing diffs and refreshing. > State.update(); > + setTimeout(update(), 1000); Not `update()`, `update`, otherwise you'll run the code immediately. ::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml @@ +87,5 @@ > <body onload="go()"> > <table id="data"> > </table> > + <input type="button" onclick="startUpdate()" value="Play"></input> > + <input type="button" onclick="stopUpdate()" value="Pause"></input> Generally, we prefer giving an `id` to the elements and calling `addEventListener` from JS code. I know, three lines above, we're violating that rules :)
Attachment #8592603 - Flags: review?(dteller) → feedback+
Yeah, those are great suggestions. I havent had the time to look at this bug again (finishing up a sprint at work). Hopefully I can implement the changes tonight.
I implemented the changes you mentioned. I still need to add the drop down menu to select the intervals and some comments to the code. Please let me know what you think.
Attachment #8592603 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8594284 - Flags: feedback?(dteller)
Probably not for this bug, but something I think would be useful is an 'update on jank level n' mode - so that the page only refreshes when something seriously janks the browser (letting you see what caused the latest instancce). Divining the desired jank level might be difficult for users though (unless there was some indication of the highest jank level seen to date). Tracking the 'maximum jank' that anything has caused, and only updating when something surpasses that maximum might be more useful, but then you'd need a way to clear it.
Emanuel, don't hesitate to file a bug.
Flags: needinfo?(dteller)
Comment on attachment 8594284 [details] [diff] [review] bug1150863_add_play_pause_button.diff Review of attachment 8594284 [details] [diff] [review]: ----------------------------------------------------------------- This looks mostly good. Can you apply the changes below? ::: toolkit/components/aboutperformance/content/aboutPerformance.js @@ +21,5 @@ > {key: "totalCPOWTime", percentOfDeltaT: true, label: "Cross-Process (%)"}, > {key: "ticks", percentOfDeltaT: false, label: "Activations"}, > ]; > > +let AutoUpdate = { This object and all its fields will need a little documentation. @@ +25,5 @@ > +let AutoUpdate = { > + > + _timerId: null, > + > + startUpdate: function(){ Now that it's in an object `AutoUpdate`, you can just call these methods `start` and `stop`. @@ +26,5 @@ > + > + _timerId: null, > + > + startUpdate: function(){ > + if (AutoUpdate._timerId != null) return; We always put braces in an `if`, to avoid accidents. @@ +31,5 @@ > + AutoUpdate._timerId = window.setInterval(update, 2000); > + }, > + > + stopUpdate: function(){ > + if (AutoUpdate._timerId != null){ Generally, we rather write if (AutoUpdate._timerId == null) { return; } clearInterval(...) this._timerId = null; @@ +34,5 @@ > + stopUpdate: function(){ > + if (AutoUpdate._timerId != null){ > + clearInterval(AutoUpdate._timerId); > + AutoUpdate._timerId = null; > + } By the way, we indent with two whitespaces. @@ +36,5 @@ > + clearInterval(AutoUpdate._timerId); > + AutoUpdate._timerId = null; > + } > + } > + Nit: Please make sure that you do not add whitespace at the end of lines. @@ +168,5 @@ > > function go() { > // Compute initial state immediately, then wait a little > // before we start computing diffs and refreshing. > + document.getElementById('playButton').addEventListener("click", AutoUpdate.startUpdate); To avoid accidents, make it `() => AutoUpdate.startUpdate()` and `() => AutoUpdate.stopUpdate()`. ::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml @@ +87,5 @@ > <body onload="go()"> > <table id="data"> > </table> > + <input type="button" id="playButton" value="Play"></input> > + <input type="button" id="pauseButton" value="Pause"></input> Can you put these buttons on the top? Otherwise, we won't see them if the list is too large.
Attachment #8594284 - Flags: feedback?(dteller) → feedback+
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #14) > Emanuel, don't hesitate to file a bug. OK, I filed bug 1156266.
Great suggestions David. I just didnt understand one of the points you mentioned. You mentioned: To avoid accidents, make it `() => AutoUpdate.startUpdate()` and `() => AutoUpdate.stopUpdate()`. I thought that: () => AutoUpdate.startUpdate() was equivalent to AutoUpdate.startUpdate(). How does wrapping it in an arrow function make it safer?
Flags: needinfo?(dteller)
There is actually a difference: let MyObject = { foo: 5, bar: function() { console.log(this.foo); } }; MyObject.bar(); // Displays 5, as intended let a = MyObject.bar; a(); // Displays undefined, because we have lost `this`. let b = () => MyObject.bar(); b(); // Displays 5, as intended The use of `() => AutoUpdate.startUpdate()` protects `this` in case we decide to use it `startUpdate`.
Flags: needinfo?(dteller)
Hey David, I implemented the changes that you suggested. Since you are away until May 6th, I have asked Avi to review the changes.
Attachment #8594284 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8598249 - Flags: review?(avihpit)
Hey Avi, I hope you are ok with reviewing my changes. You were one of the suggested reviewers. As part of this bug, there is one more addition that is yet to be implemented. So if you do review it, don't land the changes just yet.
Flags: needinfo?(avihpit)
Comment on attachment 8598249 [details] [diff] [review] bug1150863_add_play_pause_button.diff I'd Prefer David to review it.
Flags: needinfo?(avihpit)
Attachment #8598249 - Flags: review?(avihpit)
Yeah, David will be back in two days. Might as well wait until then. In the meantime, I guess I can commit the extra changes that I made so that he can do a complete review of the patch. Thanks Avi.
Flags: needinfo?(dteller)
Hey David, this patch contains the dropdown that lets users choose the refresh rate.
Attachment #8598249 - Attachment is obsolete: true
Attachment #8601248 - Flags: review?(dteller)
Comment on attachment 8601248 [details] [diff] [review] bug1150863_add_play_pause_button.diff Review of attachment 8601248 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with a few trivial changes below. Also, could you add "Bug 1150863 - " at the start of your commit message? ::: toolkit/components/aboutperformance/content/aboutPerformance.js @@ +23,5 @@ > {key: "ticks", percentOfDeltaT: false, label: "Activations"}, > ]; > > +/** > + * Nit: Please remove that empty line. @@ +24,5 @@ > ]; > > +/** > + * > + * Used to control the updates in the performance page. to control the *live* updates @@ +39,5 @@ > + */ > + _intervalDropdown: null, > + > + /** > + * Starts updating the performance data every 2 seconds if the updates are paused. Well, not necessarily every 2 seconds, right? @@ +48,5 @@ > + } > + > + if (AutoUpdate._timerId == null) { > + AutoUpdate._timerId = window.setInterval(update, > + AutoUpdate._intervalDropdown.options[AutoUpdate._intervalDropdown.selectedIndex].value); Can you cut this in several shorter lines?
Attachment #8601248 - Flags: review?(dteller) → review+
Welcome back David! I made the changes that you suggested. Now that this bug is nearing the end, I was looking for another one that I can learn from. I know that the about:performance page still has a lot of development left, so if you have anything in mind, let me know.
Attachment #8601248 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8601692 - Flags: review?(dteller)
By the way, have you checked whether it passes all tests? In particular, * the following command makes sure that the tests use the latest version of your code: ./mach build toolkit/components/aboutperformance * the following command launches the unit tests on about:performance ./mach mochitest toolkit/components/aboutperformance/tests/browser/browser_aboutperformance.js
Flags: needinfo?(wrahman0)
Yeah, they pass with the following result: TEST-PASS | leakcheck | default process: no leaks detected! runtests.py | Running tests: end. TEST-INFO | checking window state Browser Chrome Test Summary Passed: 1 Failed: 0 Todo: 0 *** End BrowserChrome Test Results *** SUITE-END | took 116s
Flags: needinfo?(wrahman0)
Would you be interested in working on Bug 1162053?
Comment on attachment 8601692 [details] [diff] [review] bug1150863_add_play_pause_button.diff Review of attachment 8601692 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but the patch doesn't apply cleanly. Can you pull the latest version and rebase your patch?
Attachment #8601692 - Flags: review?(dteller) → feedback+
Yeah, I realize that my working branch is sorta behind. I will pull and resolve the conflict tomorrow. Also, Bug 1162053 looks very interesting. I would love to work on it.
Flags: needinfo?(dteller)
I pulled the latest changes and interestingly, I was able to merge my changes (though 'hg qpush') without any conflicts. The .diff file was also unchanged. Here are the steps I followed to pull the changes: hg qpop hg pull hg update hg qpush ./mach clobber ./mach build ./mach build toolkits/components/aboutperformance/ After which I was able to run the build and see the changes applied and working correctly. Finally: hg qrefresh I ran a diff on the two patches and saw no differences. I am unfamiliar with the merge process on your end but I can't think of any steps that I may have missed. I will keep looking into it but I want to hear what you think.
Flags: needinfo?(dteller)
Mmmh... You're right, your patch seems to apply nicely, I'm not sure what happened. Here's the Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f26a631fa145
Flags: needinfo?(dteller)
Attachment #8601692 - Flags: feedback+ → review+
All clear. Let's land this.
Just curious, how did you end up fixing the issue with the try server?
Flags: needinfo?(dteller)
The issue was on my computer, not on Try.
Flags: needinfo?(dteller)
Oops, forgot to mark this checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c93e975c4ea
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: