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)

defect

Tracking

(firefox40 verified)

VERIFIED FIXED
Tracking Status
firefox40 --- verified

People

(Reporter: paul, Assigned: pbro)

References

Details

Attachments

(2 files, 3 obsolete files)

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
Blocks: perf-tool-papercuts
No longer blocks: perf-tool-v2
Summary: FxOS / Perf++: show a throbber while retrieving the profiler data → Show a throbber while retrieving the profiler data
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.
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
Should this lock the whole tool, or just the currently loading recording?
I don't think it matters much. So go for the simplest solution.
No longer blocks: perf-tool-v2
No longer blocks: perf-polish
Assignee: nobody → jsantell
Priority: -- → P1
Status: NEW → ASSIGNED
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)
omg, I don't know how to bugzilla.
Assignee: pbrosset → jsantell
Flags: needinfo?(pbrosset) → needinfo?(jsantell)
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)
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
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.
Attached patch bug1146239-loading-record.diff (obsolete) — Splinter Review
For info. Let me know if this helps or not.
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!
(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.
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)
Attached patch 1146239-l10n.patch (obsolete) — Splinter Review
Apparently string stuff is happening now and we're holding them up -- dropping this quick and pinging bgrins
Attachment #8604167 - Flags: review?(bgrinstead)
(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.
Attachment #8604167 - Flags: review?(bgrinstead)
4) sounds good -- there are many tools that have "Loading[ellipses]" actually!
Blocks: 1163763
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.
/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)
Attachment #8604536 - Flags: review?(jsantell) → review+
Keywords: leave-open
Assignee: jsantell → pbrosset
OS: Mac OS X → Unspecified
Hardware: x86 → Unspecified
Attachment #8604536 - Flags: checkin+
Attachment #8601085 - Attachment is obsolete: true
Attachment #8604167 - Attachment is obsolete: true
Blocks: perf-polish
No longer blocks: perf-tool-papercuts
(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.
No tests yet, attaching this to ask for feedback only.
Attachment #8606246 - Flags: feedback?(jsantell)
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+
Attachment #8604536 - Flags: review+ → review?(jsantell)
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/
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 on attachment 8604536 [details]
MozReview Request: bz://1146239/pbrosset

Looks good! Thanks, Patrick!
Attachment #8604536 - Flags: review+
Comment on attachment 8604536 [details]
MozReview Request: bz://1146239/pbrosset

https://reviewboard.mozilla.org/r/8639/#review7595

Ship It!
Attachment #8604536 - Flags: review+
Thanks Jordan!
You're right about the test, let's see how it behaves. I'll change it if it turns into an intermittent.
pbro, is this good to close?
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Flags: qe-verify+
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 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?
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 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+
Attachment #8604536 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attachment #8604536 - Attachment is obsolete: true
Attachment #8619838 - Flags: review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.