Closed
Bug 1146239
Opened 9 years ago
Closed 9 years ago
Show a throbber while retrieving the profiler data
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox40 verified)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
firefox40 | --- | verified |
People
(Reporter: paul, Assigned: pbro)
References
Details
Attachments
(2 files, 3 obsolete files)
5.71 KB,
patch
|
jsantell
:
feedback+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
39 bytes,
text/x-review-board-request
|
jsantell
:
review+
|
Details |
Stopping the recording takes time with Firefox OS. Several seconds. It feels like the tool is frozen. We should show a message or a throbber
Updated•9 years ago
|
Blocks: perf-tool-v2
Updated•9 years ago
|
Summary: FxOS / Perf++: show a throbber while retrieving the profiler data → Show a throbber while retrieving the profiler data
Comment 3•9 years ago
|
||
Bug regarding the clock in the details view lags behind while loading: bug 1133054 And bug 1144439 seems like a dupe of this, with additional clarification of showing it in the sidebar as well.
Updated•9 years ago
|
Blocks: perf-polish
Comment 4•9 years ago
|
||
This is important to have -- even subsequent profiles that are brief take a longer than expected time (guessing because it has to filter out all the previous samples, and bug 1145824 will help a bit, I think, but still.
Blocks: perf-tool-v2
Comment 5•9 years ago
|
||
Should this lock the whole tool, or just the currently loading recording?
Reporter | ||
Comment 6•9 years ago
|
||
I don't think it matters much. So go for the simplest solution.
Updated•9 years ago
|
No longer blocks: perf-tool-v2
Updated•9 years ago
|
No longer blocks: perf-polish
Updated•9 years ago
|
Assignee: nobody → jsantell
Priority: -- → P1
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•9 years ago
|
||
I was looking into this bug a little bit today, before realizing it was assigned to you. Let me know if you already have patches ready for this. If not, I think I know how to fix this.
Assignee: jsantell → pbrosset
Flags: needinfo?(pbrosset)
Assignee | ||
Comment 8•9 years ago
|
||
omg, I don't know how to bugzilla.
Assignee: pbrosset → jsantell
Flags: needinfo?(pbrosset) → needinfo?(jsantell)
Comment 9•9 years ago
|
||
So this should wait until bug 1159052 is finished, as that provides a nice layer of "stopping" and "stopped" events, indicating two points in time from the moment where a user clicks "stop" to stop recording, and the wait until the profiler data comes over RDP and we do any sort of massaging for it. Difficulties here are figuring out what to make inaccessible -- the whole tool? Just the recording? I'm guessing for now, maybe when selecting a recording that is not recording (`!model.isRecording()`) and not yet completed (`!model.isCompleted()`), we throw a shadow over the details/overview views with the throbber while everything populates. I do not have any patches for this, if you want to grab this, go for it :D let me know if you have any questions about it, the main goal here is to minimize lock up and provide feedback for the cases where we'll have to wait.
Flags: needinfo?(jsantell)
Comment 10•9 years ago
|
||
And not sure if this should be related to this bug, but there are several other scenarios that can cause large waits, or would benefit from a 'throb' screen. Having two large recordings, select the JS call tree, and switch between those recordings, causing the call tree to fully rerender each time, leading to long waits. Should we throb on recording switch, should we throb on any details view switch if it has to rerender? Related: bug 1128758
Assignee | ||
Comment 11•9 years ago
|
||
Thanks for the detailed explanation Jordan. (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9) > So this should wait until bug 1159052 is finished, as that provides a nice > layer of "stopping" and "stopped" events, indicating two points in time from > the moment where a user clicks "stop" to stop recording, and the wait until > the profiler data comes over RDP and we do any sort of massaging for it. When I looked at the code earlier today, it looked to me like the RECORDING_WILL_STOP and RECORDING_STOPPED events would provide exactly what you described, but I guess that's not the whole story. > Difficulties here are figuring out what to make inaccessible -- the whole > tool? Just the recording? I'm guessing for now, maybe when selecting a > recording that is not recording (`!model.isRecording()`) and not yet > completed (`!model.isCompleted()`), we throw a shadow over the > details/overview views with the throbber while everything populates. My take on fixing this was: 1. change the duration label in the recordings list from "recording..." to "loading..." while the data is being received and massaged. It's a detail, but it plays an important role in showing that something's going on. 2. add a new "loading" state in performance-view.js that shows a new simple loading screen between the WILL_STOP and STOPPED events (simple center label with the devtools-throbber next to it). > I do not have any patches for this, if you want to grab this, go for it :D > let me know if you have any questions about it, the main goal here is to > minimize lock up and provide feedback for the cases where we'll have to wait. I'll upload a small patch for what I described above, so you can tell me if I'm on the wrong path or not. I haven't followed the new perf tool ui development along closely enough, so I might be missing important things, in which case it might make more sense for you to keep the bug assigned. But anyway, since I spent some time thinking about this, I might as well attach the patch.
Assignee | ||
Comment 12•9 years ago
|
||
For info. Let me know if this helps or not.
Comment 13•9 years ago
|
||
RECORDING_WILL_STOP and RECORDING_STOPPED are correct -- the change in bug 1159052 just makes them more accurate, in a way, and also distinguishes the difference between recording and completion. Check out ./views/overview.js, as that handles any change of state on a recording, and checks to see if its the current recording, and what it should do based off of recording/completion/is-a-console-recording status, I think it'll be pretty similar Great call on changing the "recording..." label! We should add that ASAP to get strings in for 40.0. Sounds like you got the right idea -- something that I'm unsure about, and I don't think I explained it well, is if you have recording #2 selected, and is recording, and recording #1 is still recording, and then finishes, do we lock up recording #2? I think it'll lock everything, so maybe the "loading" state with the throbber can say "Fetching data for {recordingname}" (which would then stop rendering the overview for recording #2?). Maybe putting anything that locks the tool in a worker would prevent this in the first place, so we'd only need it if the current recording is being fetched. All of this would only happen if you have had a console.profile recording, which is pretty unused I'm pretty sure anyway. Thanks Patrick!
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13) > Great call on changing the "recording..." label! We should add that ASAP to > get strings in for 40.0. I was off last week unfortunately, the merge date is today, right? Am I too late to land string changes? I've prepared a patch just for this but I need to finish the test for it first.
Comment 15•9 years ago
|
||
I don't think it's too late to land string changes -- how about we just land a patch with just string changes for this to give us more time? So the .dtd has a "Loading…" entry, so if we have one for the .properties l10n file as well, I think that'll cover all of our bases with this (as I was trying to think of different stages to display, like fetching data, parsing data, etc.,) but just "Loading…" I think will cover anything we need to display in the panel, as well as injecting into the RecordingsView. R+ for that l10n change of adding just a "Loading…" entry in the profiler.properties file instead of "Loading record…" (same key name though, as I think that's the only place we'd use it dynamically, and we don't need to call it a "record" in this case)
Comment 16•9 years ago
|
||
Apparently string stuff is happening now and we're holding them up -- dropping this quick and pinging bgrins
Attachment #8604167 -
Flags: review?(bgrinstead)
Comment 17•9 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #16) > Created attachment 8604167 [details] [diff] [review] > 1146239-l10n.patch > > Apparently string stuff is happening now and we're holding them up -- > dropping this quick and pinging bgrins I think we've missed the boat on the string changes. Here are some options I can think of 1) Land the string separately via the normal route and ask flod about uplifting. I'm not sure how difficult it makes things if we uplift a string within a day of the merge, but we could check 2) Land the feature with the 'recording..' label and then switch it in a separate patch that doesn't get uplifted (for 41) 3) Land the feature with the 'loading...' string hardcoded and switch it only if the proper localization string exists (which would happen in 41 if we didn't do (1)) 4) Reuse an existing string that says 'loading...' if we have one already within the devtools strings and then land the new profiler specific one and switch to it for 41.
Updated•9 years ago
|
Attachment #8604167 -
Flags: review?(bgrinstead)
Comment 18•9 years ago
|
||
4) sounds good -- there are many tools that have "Loading[ellipses]" actually!
Assignee | ||
Comment 19•9 years ago
|
||
Thanks Brian. Like Jordan, I think that 4) sounds like a good option given our constraints. So I'm going to submit a small patch just for the records sidebar to show the "loading..." message during the WILL_STOP and the STOPPED events by reusing an existing string. That's a quick win, and it makes a difference, so let's try and get that in first and then I'll try and give more thoughts to the other part.
Assignee | ||
Comment 20•9 years ago
|
||
/r/8641 - Bug 1146239 - Show the recording as loading in the performance sidebar after it has stopped; r=jsantell Pull down this commit: hg pull -r 9926e97d7bb9da4d8f44b74740ba36d2005a5af8 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8604536 -
Flags: review?(jsantell)
Assignee | ||
Comment 21•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d015d8b8a99f
Comment 22•9 years ago
|
||
https://reviewboard.mozilla.org/r/8641/#review7249 Looks good!
Updated•9 years ago
|
Attachment #8604536 -
Flags: review?(jsantell) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Assignee: jsantell → pbrosset
OS: Mac OS X → Unspecified
Hardware: x86 → Unspecified
Assignee | ||
Updated•9 years ago
|
Attachment #8604536 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8601085 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8604167 -
Attachment is obsolete: true
Updated•9 years ago
|
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13) > something that I'm unsure about, and I > don't think I explained it well, is if you have recording #2 selected, and > is recording, and recording #1 is still recording, and then finishes, do we > lock up recording #2? It took me a while to understand that because I assumed you could only ever have one profile recording at the same time, because the UI doesn't let you have more than one. But console.profile|End does allow this indeed. > I think it'll lock everything, so maybe the "loading" > state with the throbber can say "Fetching data for {recordingname}" (which > would then stop rendering the overview for recording #2?). So, you're right, the patch I originally uploaded did lock everything up, and in fact with the scenario you described, it even failed badly. It would be easy enough to only lock the details-pane if WILL_STOP happens for the currently selected recording. If you're not using console.profile, it's not going to make a huge difference because anyway, if you have recording #1 selected and recording #2 finishes, then we automatically select #2. So even if we don't lock recording #1 while #2 is being loaded, the user can't really look at #1 anymore because #2 gets selected shortly after. > Maybe putting > anything that locks the tool in a worker would prevent this in the first > place, so we'd only need it if the current recording is being fetched. Yeah, that would be nice.
Assignee | ||
Comment 26•9 years ago
|
||
No tests yet, attaching this to ask for feedback only.
Attachment #8606246 -
Flags: feedback?(jsantell)
Comment 27•9 years ago
|
||
Comment on attachment 8606246 [details] [diff] [review] bug1146239-lock-details-perf-pane.diff Review of attachment 8606246 [details] [diff] [review]: ----------------------------------------------------------------- Ah, this is great! Simple and gets the job done perfectly. Just needs a test (and I can take care of that too) and I think it's good to go!
Attachment #8606246 -
Flags: feedback?(jsantell) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #8604536 -
Flags: review+ → review?(jsantell)
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8604536 [details] MozReview Request: bz://1146239/pbrosset /r/8641 - Bug 1146239 - Show the recording as loading in the performance details pane after it has stopped; r=jsantell Pull down this commit: hg pull -r 7e7e89e0b6400d33bc3374be31ab1a00dff33c4d https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 29•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93ff9721ce2f
Comment 30•9 years ago
|
||
Comment on attachment 8604536 [details] MozReview Request: bz://1146239/pbrosset https://reviewboard.mozilla.org/r/8639/#review7593 ::: browser/devtools/performance/test/browser_perf-loading-02.js:37 (Diff revision 2) > + "The loading-notice is shown while the record is stopping"); I wonder if this will fail occasionally on CI -- if the STOPPED event occurs immediately after WILL_STOP, the details view could already be shown. Should be fine, but in the event where this has trouble consistently passing, we could mock the events calling
Attachment #8604536 -
Flags: review?(jsantell)
Comment 31•9 years ago
|
||
Comment on attachment 8604536 [details]
MozReview Request: bz://1146239/pbrosset
Looks good! Thanks, Patrick!
Attachment #8604536 -
Flags: review+
Comment 32•9 years ago
|
||
Comment on attachment 8604536 [details] MozReview Request: bz://1146239/pbrosset https://reviewboard.mozilla.org/r/8639/#review7595 Ship It!
Attachment #8604536 -
Flags: review+
Assignee | ||
Comment 34•9 years ago
|
||
Thanks Jordan! You're right about the test, let's see how it behaves. I'll change it if it turns into an intermittent.
Assignee | ||
Comment 37•9 years ago
|
||
Yes it is, sorry I forgot to remove the leave-open flag. Marking as Resolved Fixed, not sure if there are other flags I should be changing.
Updated•9 years ago
|
Flags: qe-verify+
Comment 38•9 years ago
|
||
Comment on attachment 8604536 [details] MozReview Request: bz://1146239/pbrosset Approval Request Comment [Feature/regressing bug #]: 1167252, the new performance tool [User impact if declined]: Won't ship the performance tool [Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift [Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake. Risks are generally contained within devtools, specifically within the performance panel. [String/UUID change made/needed]: None
Attachment #8604536 -
Flags: approval-mozilla-aurora?
Comment 39•9 years ago
|
||
Comment on attachment 8606246 [details] [diff] [review] bug1146239-lock-details-perf-pane.diff Approval Request Comment [Feature/regressing bug #]: 1167252, the new performance tool [User impact if declined]: Won't ship the performance tool [Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift [Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake. Risks are generally contained within devtools, specifically within the performance panel. [String/UUID change made/needed]: None
Attachment #8606246 -
Flags: approval-mozilla-aurora?
Comment 40•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d26316f1015 https://hg.mozilla.org/releases/mozilla-aurora/rev/598885f81bb2
status-firefox40:
--- → fixed
Comment 41•9 years ago
|
||
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+. See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment 42•9 years ago
|
||
Comment on attachment 8606246 [details] [diff] [review] bug1146239-lock-details-perf-pane.diff Change approved to skip one train as part of the spring campaign.
Attachment #8606246 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8604536 -
Flags: approval-mozilla-aurora?
Comment 43•9 years ago
|
||
Verified fixed on Aurora 40.0a2 (2015-06-08), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5. A loading message along with a throbber are displayed while waiting for profile data retrieval.
Assignee | ||
Comment 44•9 years ago
|
||
Attachment #8604536 -
Attachment is obsolete: true
Attachment #8619838 -
Flags: review+
Assignee | ||
Comment 45•9 years ago
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•