New UI for the sampling Profiler

RESOLVED FIXED in Firefox 34

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: anton, Assigned: vporof)

Tracking

(Blocks 1 bug, {perf})

Trunk
Firefox 34
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 34+)

Details

(Whiteboard: [c=devtools p= s= u=][status:landedon])

Attachments

(3 attachments, 31 obsolete attachments)

303.12 KB, patch
vporof
: review+
Details | Diff | Splinter Review
171.67 KB, patch
vporof
: review+
Details | Diff | Splinter Review
208.69 KB, patch
vporof
: review+
Details | Diff | Splinter Review
This bug is about replacing Cleopatra with a new UI written from scratch. This will remove an untested chunk of code from the devtools code base and allow us to iterate quicker in future.

Right now our instance of Cleopatra has a few modifications that are unsuitable for the upstream which makes our copy a fork of the main Cleopatra project. This means that we cannot easily merge new changes from the upstream.

I think it's beneficial for the team to make our UI separate from Cleopatra and contribute to both projects (our DevTools for web devs and Cleopatra for platform/chrome hackers).

Plan:

1) Land the first prototype (not necessarily feature complete) under a flag.
2) Iterate on it until we're ready to swap Cleopatra with new UI.
3) ...
4) Profit.
I think that's fair. I've tried using the Cleopatra while putting myself in the shoes of a front-end developer and it's not easy. I think that platform and webdev have different needs so sharing the UI isn't going to work.
No longer blocks: 850145
Depends on: 850145
No longer blocks: 869245
Posted patch nocleo.patch (obsolete) — Splinter Review
WIP 1.

To see new histogram you will need to enable devtools.profiler.new-ui flag.
Summary: [meta] New UI for the sampling Profiler → New UI for the sampling Profiler
Posted patch nocleo.patch (obsolete) — Splinter Review
WIP 2 (UI stuff)
Attachment #786503 - Attachment is obsolete: true
Posted patch nocleo.patch (obsolete) — Splinter Review
New minimap in the toolbar.
Attachment #786648 - Attachment is obsolete: true
Err forgot to add: WIP 3.
Blocks: 919823
Blocks: perf-kanban
Just applied this patch onto the current tree and everything looks good. Gonna finish it up and land before my vacation days.
Posted patch WIP 4 (obsolete) — Splinter Review
Attachment #789897 - Attachment is obsolete: true
Posted patch WIP 5 (obsolete) — Splinter Review
Attachment #8341395 - Attachment is obsolete: true
Posted patch WIP 6 (obsolete) — Splinter Review
Chart is ready more-or-less. Gonna work on the tree view next.
Assignee: anton → nobody
Status: ASSIGNED → NEW
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8342748 - Attachment is obsolete: true
Tagging this so that the b2g perf team can track this work.
Keywords: perf
Whiteboard: [c=devtools p= s= u=]
Blocks: 869245
Depends on: 1007200
Depends on: 1007202
Depends on: 1007203
Duplicate of this bug: 823026
Duplicate of this bug: 834880
Duplicate of this bug: 853653
Duplicate of this bug: 857292
No longer depends on: 1007200
No longer blocks: 818751, 823026, 828038, 834880, 845752, 853653, 857292, 869245
No longer depends on: 850145, 879007
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1007202
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attachment #8342074 - Attachment is obsolete: true
No longer blocks: 834186
No longer blocks: 840235
Depends on: 1007460
Depends on: 1007461
Depends on: 1007463
No longer depends on: 1007203
Duplicate of this bug: 1014461
Is there flag in about:config for externals to test the new profiler ?
It's neither ready nor landed yet.
Depends on: 1023018
Depends on: 1023441
Depends on: 1024633
OS: Mac OS X → All
Hardware: x86 → All
Posted patch v1 (wip) - remove everything (obsolete) — Splinter Review
Posted patch v1 (wip) - reboot (obsolete) — Splinter Review
Posted image screenshot (wip) (obsolete) —
Try builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-efdda927cf43/

Lots of stuff isn't finished (like import/export, filtering by selection etc.), but the basic functionality is there, console.profile|profileEnd works too.
(In reply to Victor Porof [:vporof][:vp] from comment #23)
> Try builds:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.
> com-efdda927cf43/
> 
> Lots of stuff isn't finished (like import/export, filtering by selection
> etc.), but the basic functionality is there, console.profile|profileEnd
> works too.

Oh yeah baby, that is beautiful. Nice work, Victor!
Posted image screenshot (v2 wip) (obsolete) —
New screenshot showcasing filtering and a nicer tree view. Patches and builds on their way.
Attachment #8440442 - Attachment is obsolete: true
Attachment #8440443 - Attachment is obsolete: true
Attachment #8440444 - Attachment is obsolete: true
Awesome! Looking wonderful :)

Questions:

* Why the change to "Snapshot"? A snapshot is an instance in time, while these profiles are recordings over a period of time. How about "Recording"?

* When this lands, can we rename the tab from "Profiler" to "Performance"? This tool is evolving to be everything performance related, not just a JS profiler.
Posted patch v2 (wip) - remove everything (obsolete) — Splinter Review
Posted patch v2 (wip) - reboot (obsolete) — Splinter Review
(In reply to Nick Fitzgerald [:fitzgen] from comment #26)
> Awesome! Looking wonderful :)
> 
> Questions:
> 
> * Why the change to "Snapshot"? A snapshot is an instance in time, while
> these profiles are recordings over a period of time. How about "Recording"?
> 
> * When this lands, can we rename the tab from "Profiler" to "Performance"?
> This tool is evolving to be everything performance related, not just a JS
> profiler.

Both suggestions sound good to me!
Just played around with it for a few minutes, my small pieces of feedback:

* It wasn't intuitive to me to press "+" to zoom in. When I did, it creates little tabs that automatically disappear when you click back (by clicking a more zoomed out tab). The tabs are neat, but it might be more intuitive to make the + more prominent somehow, and only support going back to the base zoom level, which means you just need another "reset zoom" button.

* Having both the FPS and frame graph open takes up quite a lot of vertical space, and I feel like personally I only want to look at FPS every now and then. What do you think about adding a way to show/hide the FPS or something?

I haven't been following this too closely so not too aware of current conversations around it. It looks great!
> takes up quite a lot of vertical space

Maybe we can just shorten the height?
(In reply to James Long (:jlongster) from comment #31)
> Just played around with it for a few minutes, my small pieces of feedback:
> 
> * It wasn't intuitive to me to press "+" to zoom in. When I did, it creates
> little tabs that automatically disappear when you click back (by clicking a
> more zoomed out tab). The tabs are neat, but it might be more intuitive to
> make the + more prominent somehow, and only support going back to the base
> zoom level, which means you just need another "reset zoom" button.
> 

Agreed that + isn't very discoverable. Need some UI/UX feedback/suggestions on that.

Regarding going back, we could just have close icons on the tabs.

> * Having both the FPS and frame graph open takes up quite a lot of vertical
> space, and I feel like personally I only want to look at FPS every now and
> then. What do you think about adding a way to show/hide the FPS or something?
> 
> I haven't been following this too closely so not too aware of current
> conversations around it. It looks great!

Adding buttons to collapse them might be a good idea.
(In reply to Victor Porof [:vporof][:vp] from comment #33)
> (In reply to James Long (:jlongster) from comment #31)
> > Just played around with it for a few minutes, my small pieces of feedback:
> > 
> > * It wasn't intuitive to me to press "+" to zoom in. When I did, it creates
> > little tabs that automatically disappear when you click back (by clicking a
> > more zoomed out tab). The tabs are neat, but it might be more intuitive to
> > make the + more prominent somehow, and only support going back to the base
> > zoom level, which means you just need another "reset zoom" button.
> > 
> 
> Agreed that + isn't very discoverable. Need some UI/UX feedback/suggestions
> on that.
> 
> Regarding going back, we could just have close icons on the tabs.

What do the tabs give us that the click+drag selection doesn't?
 
> > * Having both the FPS and frame graph open takes up quite a lot of vertical
> > space, and I feel like personally I only want to look at FPS every now and
> > then. What do you think about adding a way to show/hide the FPS or something?
> > 
> > I haven't been following this too closely so not too aware of current
> > conversations around it. It looks great!
> 
> Adding buttons to collapse them might be a good idea.

+1
(In reply to Nick Fitzgerald [:fitzgen] from comment #34)
> 
> What do the tabs give us that the click+drag selection doesn't?
>  

A zoomed-in selection.
Posted patch remove everything (obsolete) — Splinter Review
Attachment #8441699 - Attachment is obsolete: true
Posted patch v3 (wip) - reboot (obsolete) — Splinter Review
Attachment #8441700 - Attachment is obsolete: true
Depends on: 1029097
Depends on: 1029540
Duplicate of this bug: 1007463
Posted patch v4 (reboot) (obsolete) — Splinter Review
Cleaned up and documented everything.
Attachment #8441664 - Attachment is obsolete: true
Attachment #8444606 - Attachment is obsolete: true
Attachment #8444604 - Attachment description: v3 (wip) - remove everything → remove everything
I spent some time in Chrome's profiler last night and I thought of one more piece of feedback. I really how Chrome plots the time of the profile across the top of the graph (instead of just showing 0ms -> 1000 ms like we do, and creating tabs). When you zoom in, these values just spread out more, and the values on the far left and far right show your bounds, with points plotted between them so you can look at a point in the graph and easily see when it happened.

I'd rather see more of an x-axis plot of time rather than the tabs, personally. It really reinforces the zooming in behavior.
(In reply to James Long (:jlongster) from comment #41)

We'll have that in the flame graph view.
(In reply to Victor Porof [:vporof][:vp] from comment #42)
> (In reply to James Long (:jlongster) from comment #41)
> 
> We'll have that in the flame graph view.

Woot, thanks.

Victor and I talked on IRC and I mentioned that the "JS" category was confusing to me at first. I didn't know it was only JS compilation time and not execution time (which makes sense now, but I guarantee that will make people think we are also recording JS exec time).

Victor proposed changing it to "JIT" which I think is brilliant and instantly makes me understand that graph better.
Regarding determining if JS is taking time, or the C++ calling that JS (from IRC): If JS is on the top of the stack (above the C++), couldn't we categorize it as JS execution time?

+1 to JIT

Also need to split "graphics" into "Layout" and "Graphics" when possible. Can't differentiate reflows vs paints vs webgl at the moment.
(In reply to Nick Fitzgerald [:fitzgen] from comment #44)
> 
> Also need to split "graphics" into "Layout" and "Graphics" when possible.
> Can't differentiate reflows vs paints vs webgl at the moment.

Completely agreed, and we'll need to instrument more platform code to make that happen. I talked to Ehsan a bit about this and doing it for the DOM would be a good way to go forward, since everything is mostly WebIDL right now and we can make the code generation scripts to also generate instrumentation. However, we need to make sure that's not actually slowing down content!
New builds, get them while they're hot! https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-609c9c54940f/

Changes:
1. Profiler now works in the Browser Toolbox too
2. Import/export implemented
3. Focusing nodes in the call tree now highlights the corresponding areas in the categories graph
3.1 Don't forget to experiment with enabling "Show Gecko platform data" in the options panel
4. Clicking on URLs jumps to the Debugger
5. Clicking on the 
Bugzilla ate half my comment because of a UTF8 character! What the hell?
Other half:

5. Clicking on the [[magnifying glass]] icon for each call node spawns a new tab marking it as root
6. Having the Gecko Profiler addon won't make our Profiler lose data
6.1 Fun fact, if you have the addon installed in Nightly, our current Profiler doesn't work anymore!
7. Renamed JavaScript to JIT
8. More obvious highlighting for the "+" (spawn tab from selection) button
8.1 Didn't add close buttons on tabs yet because click events don't work on elements inside a <tab> inside a <tabbox> and also did I mention that XUL brings me pain?
Attachment #8445615 - Attachment is obsolete: true
Great job Victor! This is looking awesome.

One last idea: don't know how easy it is, but if what if when you hover over a item in the key (network/jit/etc) it greys out all all the other types in the graph and only the one you are hovering is colored? The colors are good, but with that many sometimes it can be hard to track.

This is really coming along though!
(In reply to James Long (:jlongster) from comment #48) 
> One last idea: don't know how easy it is, but if what if when you hover over
> a item in the key (network/jit/etc) it greys out all all the other types in
> the graph and only the one you are hovering is colored? The colors are good,
> but with that many sometimes it can be hard to track.

I liked this idea so much that I already implemented it :)

New builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-001b03b804d2/
Try: https://tbpl.mozilla.org/?tree=Try&rev=001b03b804d2

If there are no other suggestions (or bugs) I'm going to focus on finishing the tests and land this.
That is hot! Awesome! We can focus on bugfixing now. :) I did seem to get it into a state where when I hovered out of the graph some of the bars were still greyed out, fwiw. Not sure if you've seen that.
Yup, I have! Consider that fixed ;)
Posted image screencap.gif (obsolete) —
Clicking on legend items now makes a large-enough selection in the graph to contain all the bars containing the respective blocks.
(not obvious from the gif, but that will also filter the call tree to match the selection bounds, just like with all selections)
Awesome ! I noticed that the graph isn't friendly to the dark theme. Otherwise for the sampling tabs, you might want to use the breadcrumb widget, and put the "+" button on the left of the toolbar.
Yes, the graph canvases aren't theme-sensitive yet. I vote for handling that in a followup bug, if people are ok with it.
For the first release, here are the things you might want to at least fix :
- outset borders on the sampling tabs (on Windows)
- black text in dark theme for the table
Thanks! I haven't tried it on Windows yet. Seems like we have a few style properties to add there.
Wow, this really looks quite horrible on Linux and Windows!
Posted patch v5 (reboot) (obsolete) — Splinter Review
Safekeeping WIP patch with no test.
Attachment #8447297 - Attachment is obsolete: true
Posted patch v6 (wip) (obsolete) — Splinter Review
FWIW, I made the tree mechanism completely generic, based on the ongoing discussion in bug 993014.

Whoever is interested, take a look at AbstractTreeItem inside utils/tree.js.

In the same file, CallView is an example implementation of an AbstractTreeItem.
Attachment #8447371 - Attachment is obsolete: true
Blocks: 1031692
Posted patch v7 (wip) (obsolete) — Splinter Review
Moved the AbstractTree implementation to its own file.
Attachment #8447466 - Attachment is obsolete: true
Comment on attachment 8447664 [details] [diff] [review]
v7 (wip)

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

Just wanted to add a few comments about the CSS, nothing very important :)

::: browser/themes/shared/devtools/profiler.inc.css
@@ +10,5 @@
> +  font-size: 120%;
> +}
> +
> +.theme-dark .notice-container {
> +  background: url(background-noise-toolbar.png), #343c45; /* Toolbars */

This file doesn't exist anymore.

@@ +15,5 @@
> +  color: #f5f7fa; /* Light foreground text */
> +}
> +
> +.theme-light .notice-container {
> +  background: url(background-noise-toolbar.png), #f0f1f2; /* Toolbars */

Same here

@@ +29,5 @@
> +}
> +
> +#empty-notice button[checked],
> +#recording-notice button[checked] {
> +  list-style-image: url(profiler-stopwatch-checked.svg);

You might want to copy the selectors into toolbars.inc.css to the filter: none rule, so that the icon isn't inverted when checked in light theme.

@@ +59,5 @@
> +}
> +
> +.theme-dark #recordings-pane > tabs,
> +.theme-dark #recordings-pane .devtools-toolbar {
> +  -moz-border-end-color: black; /* Match the splitter color. */

You might just want to use /* Splitters */ as comment here, just to be consistent with other files.

@@ +64,5 @@
> +}
> +
> +.theme-light #recordings-pane > tabs,
> +.theme-light #recordings-pane .devtools-toolbar {
> +  -moz-border-end-color: #aaa; /* Match the splitter color. */

Same here.

@@ +72,5 @@
> +  list-style-image: url(profiler-stopwatch.svg);
> +}
> +
> +#record-button[checked] {
> +  list-style-image: url(profiler-stopwatch-checked.svg);

Same as the other inverted comment.

@@ +118,5 @@
> +  font: inherit;
> +  -moz-box-align: stretch;
> +}
> +
> +#profile-content tab {

You might want to add background-color:transparent; and border:none; to fix issues on Windows.

@@ +138,5 @@
> +}
> +
> +.theme-dark #profile-content tab {
> +  -moz-appearance: none;
> +  -moz-border-end: 1px solid #000;

You should add /* Splitters */ as comment here.

@@ +143,5 @@
> +}
> +
> +.theme-light #profile-content tab {
> +  -moz-appearance: none;
> +  -moz-border-end: 1px solid #aaa;

Same here

@@ +170,5 @@
> +  transform: translateZ(1px); /* Make sure the tabpanel appears above the tab */
> +}
> +
> +#profile-newtab-button {
> +  -moz-appearance: none;

You might want to add background-color:transparent; and border:none; to fix issues on Windows.

@@ +382,5 @@
> +  display: none;
> +}
> +
> +.call-tree-zoom {
> +  -moz-appearance: none;

You might also want to add background-color:transparent too. Otherwise, you'll get an unwanted white background on Windows.

@@ +412,5 @@
> +}
> +
> +.call-tree-item:hover .call-tree-zoom {
> +  transition: opacity 0.3s ease-in;
> +  opacity: 1.0;

Not sure why .0 is needed here.

@@ +416,5 @@
> +  opacity: 1.0;
> +}
> +
> +.call-tree-item:hover .call-tree-zoom:hover {
> +  opacity: 0.0;

Same here
(In reply to Tim Nguyen [:ntim] from comment #62)
> Comment on attachment 8447664 [details] [diff] [review]
> v7 (wip)
> 

Fantastic! Thanks for looking at this Tim.
I might have downloaded an older build from the link that Jeff sent us:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-fd0950af7fb0/

This looks so much better than the previous profiler! Yay! But:

- I'd love to be able to click on one of the bars that took longer and know what happened there. I guess this is meant to be done with that sort of "range" tool, but just clicking on the bar would be cool.
- I would want to know what the memory consumption is. GC-spent time is small compared to the rest in the test I made so I can't really see how well my code is doing (or not) when looking at the bars.
- Does "graphics" include WebGL code too? What does it actually mean?

Hope that is interesting feedback!
(In reply to Soledad Penades [:sole] [:spenades] from comment #64)
> I might have downloaded an older build from the link that Jeff sent us:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.
> com-fd0950af7fb0/
> 

Thanks Sole! Yeah, the builds are a bit old and there have been a few improvements since. See comment 46 and 47 for an almost complete list.

> This looks so much better than the previous profiler! Yay! But:
> 
> - I'd love to be able to click on one of the bars that took longer and know
> what happened there. I guess this is meant to be done with that sort of
> "range" tool, but just clicking on the bar would be cool.

Hmm, do you mean that click&drag to select isn't enough? It's going to be quite tricky to do click-to-select-bar, because then you can't actually remove a previous selection anymore (if any).

> - I would want to know what the memory consumption is. GC-spent time is
> small compared to the rest in the test I made so I can't really see how well
> my code is doing (or not) when looking at the bars.

Yup, we've been thinking a lot about this. Including a memory usage timeline in this Performance tool is probably a good idea, but we're also going to have a dedicated tab for analyzing memory allocations *and* usage. As a prototype, I'll try using Panos' memory actor and see how it looks.

> - Does "graphics" include WebGL code too? What does it actually mean?
> 

It's tricky. It includes layout and rendering, which is both the 2D backends (skia, direct2d, whatever), or 3D (opengl, which includes webgl). In the long run, we'll want to take "layout" out of "graphics", and have a separate "DOM" category, but the tricky part is not actually slowing down content by annotating the platform.

> Hope that is interesting feedback!

For sure!
Posted patch v8 (wip) (obsolete) — Splinter Review
This should fix the UI issues on Windows and Linux.
Attachment #8447664 - Attachment is obsolete: true
Depends on: 1034648
Depends on: 1034651
Depends on: 1034661
Depends on: 1034668
Depends on: 1034669
Depends on: 1034664
Depends on: 1034670
Posted patch v9 (obsolete) — Splinter Review
I cut out as much as I could from the previous patches into various small(er) bugs, leaving here only the core new profiler frontend implementation. See the blocking bugs for a full list.

New builds are available here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-55815405b8e4/

In the interest of landing this asap, I suggest reviewing this patch (modulo tests, which will follow sometimes this week).
Attachment #8451745 - Flags: review?(rcampbell)
Attachment #8449683 - Attachment is obsolete: true
Attachment #8444604 - Flags: review?(rcampbell)
Comment on attachment 8444604 [details] [diff] [review]
remove everything

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

ship it.
Attachment #8444604 - Flags: review?(rcampbell) → review+
Reordering the flamechart dependency to blocked on this bug.

bug 1007461
Blocks: 1007461
No longer depends on: 1007461
Status: REOPENED → ASSIGNED
It was brought to my attention that OS X opt builds are missing from the link in comment 67. I'm blaming yesterday's red flaky trees.

Today should be fine: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-5ad521cb8670/
Took this for a test spin. The UI looks much better than cleopatra. Very pleasant in fact!

Right now you show a flamegraph which includes the platform data but the platform data isn't visible by default in the tree view. This is very confusing. You have to know to click on 'Gear' and check 'Show platform data'. Most users wont discover that. IMO we need to:

1) Show the hidden platform as a general 'Browser code' if that option isn't selected.
2) Show an option directly in the UI to display platform data. Perhaps along side the new 'Browser code'.

* Also I'm seeing dips in the FPS but I don't see any activity corresponding to it. We need to show something friendly to the user there.
* When showing platform data, I see a 100% in XRE_Main and the sum of the child is about 1.5% so we're loosing 98.5% of our execution time between that. We need a better way for the user to discover that a large segment of that time is idle or running code that isn't instrumented.
(In reply to Benoit Girard (:BenWa) from comment #71)
> 
> 1) Show the hidden platform as a general 'Browser code' if that option isn't
> selected.
> 2) Show an option directly in the UI to display platform data. Perhaps along
> side the new 'Browser code'.
> 

Yup, this is something I've been thinking of a lot lately. The best approach here might be to just collapse platform frames into generic [browser doing this kind of stuff] nodes in the tree.

> * Also I'm seeing dips in the FPS but I don't see any activity corresponding
> to it. We need to show something friendly to the user there.

Like what?

> * When showing platform data, I see a 100% in XRE_Main and the sum of the
> child is about 1.5% so we're loosing 98.5% of our execution time between
> that. We need a better way for the user to discover that a large segment of
> that time is idle or running code that isn't instrumented.

I could create an extra (idle) node that fills out the remaining percentage. Do you think that would be a good idea.
(In reply to Victor Porof [:vporof][:vp] from comment #72)
> Yup, this is something I've been thinking of a lot lately. The best approach
> here might be to just collapse platform frames into generic [browser doing
> this kind of stuff] nodes in the tree.

Sounds great.
> 
> > * Also I'm seeing dips in the FPS but I don't see any activity corresponding
> > to it. We need to show something friendly to the user there.
> 
> Like what?

Maybe I can show you tomorrow.

> 
> > * When showing platform data, I see a 100% in XRE_Main and the sum of the
> > child is about 1.5% so we're loosing 98.5% of our execution time between
> > that. We need a better way for the user to discover that a large segment of
> > that time is idle or running code that isn't instrumented.
> 
> I could create an extra (idle) node that fills out the remaining percentage.
> Do you think that would be a good idea.

Yes, as long as we also have another (Unclassified) node.
let's do those in some quick follow-ups. Already reviewing this patch and don't want it to change!
(In reply to Rob Campbell [:rc] (:robcee) from comment #74)
> let's do those in some quick follow-ups. Already reviewing this patch and
> don't want it to change!

Definitely agree with this.
My feedback was not aimed at slowing down the landing in any way. Land away!
Comment on attachment 8451745 [details] [diff] [review]
v9

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

Some comment nits, some questions and a couple of function expressions that should probably be changed to plain'ol' functions. Otherwise this looks solid to me.

I'm going to pass this on to Patrick and Nick who graciously volunteered to review this.

::: browser/devtools/framework/gDevTools.jsm
@@ +904,5 @@
>     */
> +  _connectToProfiler: function DT_connectToProfiler(event, toolbox) {
> +    let { SharedProfilerConnection } = devtools.require("devtools/profiler/shared");
> +    let connection = SharedProfilerConnection.forToolbox(toolbox);
> +    connection.open();

this is certainly cleaner than it was.

::: browser/devtools/profiler/panel.js
@@ +36,5 @@
> +    this.panelWin.gFront = new ProfilerFront(connection);
> +    yield this.panelWin.startupProfiler();
> +
> +    this.isReady = true;
> +    this.emit("ready");

why not return a promise here?

(I can't believe I'm saying this)

@@ +51,5 @@
> +      return;
> +    }
> +
> +    yield this.panelWin.shutdownProfiler();
> +    this.emit("destroyed");

ditto.

::: browser/devtools/profiler/profiler.js
@@ +13,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> +Cu.import("resource:///modules/devtools/SideMenuWidget.jsm");
> +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> +Cu.import("resource:///modules/devtools/Graphs.jsm");

can we not require() these devtools modules or are you doing this because you haven't added them to the loader yet?

@@ +34,5 @@
> +const CALL_VIEW_FOCUS_EVENTS_DRAIN = 10; // ms
> +const GRAPH_SCROLL_EVENTS_DRAIN = 50; // ms
> +const GRAPH_ZOOM_MIN_TIMESPAN = 20; // ms
> +
> +// This identifier string is simply used to tentatively ascertain whether or not

ditch "is simply used to". Maybe reword "to tentatively ascertain" to "attempts to assess". Feels wordy!

@@ +350,5 @@
> +
> +  /**
> +   * The click listener for the "import" button in this container.
> +   */
> +  _onImportButtonClick: function() {

what no Task.async(function*() ??

(just kidding, trying to stay awake)

@@ +452,5 @@
> +    this._onGraphMouseUp = this._onGraphMouseUp.bind(this);
> +    this._onGraphScroll = this._onGraphScroll.bind(this);
> +    this._onCallViewFocus = this._onCallViewFocus.bind(this);
> +    this._onCallViewLink = this._onCallViewLink.bind(this);
> +    this._onCallViewZoom = this._onCallViewZoom.bind(this);

many bind().

@@ +1144,5 @@
> +/**
> + * DOM query helpers.
> + */
> +function $(selector, target = document) target.querySelector(selector);
> +function $all(selector, target = document) target.querySelectorAll(selector);

FAIL for using function expressions. R-.

::: browser/devtools/profiler/utils/global.js
@@ +28,5 @@
> +];
> +
> +/**
> + * Mapping from category bitmasks in the profiler data to additional details.
> + * To be kept in sync with the js::ProfileEntry::Category in ProfilingStack.h

would like a forward reference to this file in ProfilingStack.h's Category comment.

@@ +31,5 @@
> + * Mapping from category bitmasks in the profiler data to additional details.
> + * To be kept in sync with the js::ProfileEntry::Category in ProfilingStack.h
> + */
> +const CATEGORY_MAPPINGS = {
> +  "4": CATEGORIES[0],

some comments pointing to the name of these categories might be useful for people trying to connect the dots.

@@ +43,5 @@
> +  "1024": CATEGORIES[7],
> +};
> +
> +// Human-readable JIT category bitmask.
> +const CATEGORY_JIT = 16;

why's this special?

::: browser/devtools/profiler/utils/tree.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";

oh goody, another "tree" widget!

Actually, as we discussed today, we should consider renaming this to something like TreeModel.js since this is building the underlying tree data-structures from the profiler data.

@@ +140,5 @@
> +
> +FrameNode.prototype = {
> +  /**
> +   * Adds function calls in the tree from a sample's frames. For example, given
> +   * the the frames below (which would account for three calls to `insert` on

extra "the".

::: browser/devtools/shared/widgets/AbstractTreeView.jsm
@@ +12,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["AbstractTreeItem"];
> +
> +/**
> + * A very generic and low-level tree view implementation. It is not indended

inTended.

@@ +102,5 @@
> + *     ▶ lazily-added-node
> + *   ▼ bar
> + *     ▶ baz
> + *
> + * Everything else is taken care of automagically!

I'm skeptical...

::: toolkit/devtools/server/actors/profiler.js
@@ +32,5 @@
> +    }
> +    this.onStopProfiler();
> +    this._profiler = null;
> +    this._observedEvents = null;
> +  },

this file feels like it didn't need to be removed. Changes are incremental enough that we could have kept the original and just included a diff here.

@@ +54,5 @@
> +    gProfilerStartTime = Date.now();
> +    this._started = true;
> +    this._checkStartedProfilers();
> +    return { started: true };
> +  },

or maybe not!

Are we worried about breaking compatibility with our previous actor here?
Attachment #8451745 - Flags: review?(rcampbell)
Attachment #8451745 - Flags: review?(pbrosset)
Attachment #8451745 - Flags: review?(nfitzgerald)
(In reply to Rob Campbell [:rc] (:robcee) from comment #77)
> Comment on attachment 8451745 [details] [diff] [review]
> v9
> 
> Review of attachment 8451745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thank you!

> 
> ::: browser/devtools/profiler/panel.js
> @@ +36,5 @@
> > +    this.panelWin.gFront = new ProfilerFront(connection);
> > +    yield this.panelWin.startupProfiler();
> > +
> > +    this.isReady = true;
> > +    this.emit("ready");
> 
> why not return a promise here?
> 

We actually do. That's what Task.async does.
Just like every other tool, the "ready" event is emitted for the toolbox, because it needs it.

> @@ +51,5 @@
> > +      return;
> > +    }
> > +
> > +    yield this.panelWin.shutdownProfiler();
> > +    this.emit("destroyed");
> 
> ditto.

Ditto.

> 
> ::: browser/devtools/profiler/profiler.js
> @@ +13,5 @@
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> > +Cu.import("resource:///modules/devtools/SideMenuWidget.jsm");
> > +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> > +Cu.import("resource:///modules/devtools/Graphs.jsm");
> 
> can we not require() these devtools modules or are you doing this because
> you haven't added them to the loader yet?
> 

There is A LOT of stuff that can't be required yet, AFAIK.

> @@ +350,5 @@
> > +
> > +  /**
> > +   * The click listener for the "import" button in this container.
> > +   */
> > +  _onImportButtonClick: function() {
> 
> what no Task.async(function*() ??
> 

Not needed, there's no asyncness in there.

> 
> ::: browser/devtools/profiler/utils/global.js
> @@ +28,5 @@
> > +];
> > +
> > +/**
> > + * Mapping from category bitmasks in the profiler data to additional details.
> > + * To be kept in sync with the js::ProfileEntry::Category in ProfilingStack.h
> 
> would like a forward reference to this file in ProfilingStack.h's Category
> comment.
> 

Good idea.

> 
> @@ +43,5 @@
> > +  "1024": CATEGORIES[7],
> > +};
> > +
> > +// Human-readable JIT category bitmask.
> > +const CATEGORY_JIT = 16;
> 
> why's this special?
> 

Because EnterJIT pseuod-frames don't actually have a category, since they're not real frames on the stack. Maybe I should add this as a comment here.

> 
> ::: toolkit/devtools/server/actors/profiler.js
> @@ +32,5 @@
> > +    }
> > +    this.onStopProfiler();
> > +    this._profiler = null;
> > +    this._observedEvents = null;
> > +  },
> 
> this file feels like it didn't need to be removed. Changes are incremental
> enough that we could have kept the original and just included a diff here.
> 

Maybe. Do you want me to split this into a separate patch?

> @@ +54,5 @@
> > +    gProfilerStartTime = Date.now();
> > +    this._started = true;
> > +    this._checkStartedProfilers();
> > +    return { started: true };
> > +  },
> 
> or maybe not!
> 
> Are we worried about breaking compatibility with our previous actor here?

There's no other consumer! The Gecko profiler isn't using it.
(In reply to Victor Porof [:vporof][:vp] from comment #78)

> > what no Task.async(function*() ??
> 
> Not needed, there's no asyncness in there.

sorry, I was just kidding.

> > > +// Human-readable JIT category bitmask.
> > > +const CATEGORY_JIT = 16;
> > 
> > why's this special?
> 
> Because EnterJIT pseuod-frames don't actually have a category, since they're
> not real frames on the stack. Maybe I should add this as a comment here.

yeah, might help explain.

> > ::: toolkit/devtools/server/actors/profiler.js
> > @@ +32,5 @@
> > > +    }
> > > +    this.onStopProfiler();
> > > +    this._profiler = null;
> > > +    this._observedEvents = null;
> > > +  },
> > 
> > this file feels like it didn't need to be removed. Changes are incremental
> > enough that we could have kept the original and just included a diff here.
> > 
> 
> Maybe. Do you want me to split this into a separate patch?
> 
> > @@ +54,5 @@
> > > +    gProfilerStartTime = Date.now();
> > > +    this._started = true;
> > > +    this._checkStartedProfilers();
> > > +    return { started: true };
> > > +  },
> > 
> > or maybe not!
> > 
> > Are we worried about breaking compatibility with our previous actor here?
> 
> There's no other consumer! The Gecko profiler isn't using it.

What about previous versions of b2g / android / firefox?
(In reply to Rob Campbell [:rc] (:robcee) from comment #79)
> 
> What about previous versions of b2g / android / firefox?

Good point. They should work, because tests, but I'll manually verify. If they don't, I'll add more tests.
Posted patch v10 (obsolete) — Splinter Review
Addressed Rob's comments.
Posted patch v10 (obsolete) — Splinter Review
Whoops, forgot the hg add the renamed tree.js
Attachment #8453268 - Attachment is obsolete: true
(In reply to Victor Porof [:vporof][:vp] from comment #78)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #77)
> > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > > +Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> > > +Cu.import("resource:///modules/devtools/SideMenuWidget.jsm");
> > > +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> > > +Cu.import("resource:///modules/devtools/Graphs.jsm");
> > 
> > can we not require() these devtools modules or are you doing this because
> > you haven't added them to the loader yet?
> > 
> 
> There is A LOT of stuff that can't be required yet, AFAIK.

That's true, but fortunately DevToolsUtils has a split js/jsm personality. I have no opinion on whether you should bother though.
Comment on attachment 8453270 [details] [diff] [review]
v10

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

Looks good! Just a few comments: mostly nits + splitting up files + more lazy loading so that we are only ever using the resources we actually need at the moment.

r+ with changes requested below.

::: browser/devtools/profiler/profiler.js
@@ +5,5 @@
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +const require = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools.require;
> +const promise = require("sdk/core/promise");

Shouldn't we be using Promise.jsm? Or do sdk promises wrap Promise.jsm now?

Can this be lazily loaded with:

    devtools.lazyRequireGetter(this, "promise", "sdk/core/promise");

?

@@ +13,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> +Cu.import("resource:///modules/devtools/SideMenuWidget.jsm");
> +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> +Cu.import("resource:///modules/devtools/Graphs.jsm");

Can we lazily load all of these? I see Task and (of course) XPCOMUtils is used immediately, but are all the rest as well?

@@ +23,5 @@
> +  "resource://gre/modules/NetUtil.jsm");
> +
> +const {L10N, CATEGORIES, CATEGORY_MAPPINGS} = require("devtools/profiler/global");
> +const {ThreadNode, CallView} = require("devtools/profiler/tree-model");
> +const {FramerateFront} = require("devtools/server/actors/framerate");

Can we lazily load these as well? lazyRequireGetter won't work for these b/c of destructuring, unfortunately, but it isn't much more work to just define a getter directly or helper for lazily getting properties from a module. Might be best just to add an optional fourth parameter to lazyRequireGetter that is a property name, so you could do:

    devtools.lazyRequireGetter(this, "L10N", "devtools/profiler/categories", "L10N");

etc.

@@ +44,5 @@
> +
> +// The panel's window global is an EventEmitter firing the following events:
> +const EVENTS = {
> +  // When a source is shown in the JavaScript Debugger at a specific location.
> +  SOURCE_SHOWN_IN_JS_DEBUGGER: "CanvasDebugger:SourceShownInJsDebugger",

JavaScript debugger or canvas debugger?

@@ +155,5 @@
> +
> +/**
> + * Functions handling the recordings UI.
> + */
> +let RecordingsListView = Heritage.extend(WidgetMethods, {

Can we split out each view into its own file and just require them when needed here? Would greatly improve navigation and readability for the profiler as this file is already getting pretty big.

Since everything is in the omnijar which is in memory, this doesn't incur any extra file I/O. Since devtools modules are all loaded in the same global now, it isn't extra memory overhead from extra compartments anymore either.

Clear win.

::: browser/devtools/profiler/utils/shared.js
@@ +38,5 @@
> + * A connection to the profiler actor, along with other miscellaneous actors,
> + * shared by all tools in a toolbox.
> + *
> + * Use `SharedProfilerConnection.forToolbox` to make sure you get the same
> + * instance every time.

Should we just export the factory function, and not the constructor? Then we wouldn't need to tell people not to foot-gun, they just couldn't do it (unless they really tried to and used instance.constructor or Object.getConstructorOf or whatever its called but at that point people know they're doing something wrong and its not an accident anymore).

So, we would move `SharedProfilerConnection.forToolbox` to `exports.getProfilerConnection` or whatever.

::: browser/devtools/profiler/utils/tree-model.js
@@ +6,5 @@
> +const {Cc, Ci, Cu, Cr} = require("chrome");
> +const Services = require("Services");
> +const {L10N, CATEGORY_MAPPINGS, CATEGORY_JIT} = require("devtools/profiler/global");
> +const {Heritage} = Cu.import("resource:///modules/devtools/ViewHelpers.jsm", {});
> +const {AbstractTreeItem} = Cu.import("resource:///modules/devtools/AbstractTreeView.jsm", {});

This one could be imported lazily.

@@ +242,5 @@
> + *        Details about this function, like { duration, invocation, calls } etc.
> + * @param number level
> + *        The indentation level in the call tree. The root node is at level 0.
> + */
> +function CallView({ caller, frame, level }) {

Yeah, let's please make this its own file and require where needed. We'll get much better navigation; I'm already confused that CallView is inside tree-model.js!

@@ +260,5 @@
> +   * @param nsIDOMNode document
> +   * @param nsIDOMNode arrowNode
> +   * @return nsIDOMNode
> +   */
> +  _displaySelf: function(document, arrowNode) {

Holy moly, this function is huge. Luckily, your comments describing what each "section" is doing seem like perfect places to break the function up into smaller ones!

::: browser/devtools/shared/widgets/AbstractTreeView.jsm
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Let's rename this file to AbstractTreeItem.jsm since it only defines and exports one thing: AbstractTreeItem.

@@ +7,5 @@
> +
> +const Cu = Components.utils;
> +
> +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> +Cu.import("resource://gre/modules/devtools/event-emitter.js");

mek dem lzy

@@ +18,5 @@
> + * own custom implementation.
> + *
> + * Language:
> + *   - An "item" is an instance of an AbstractTreeItem.
> + *   - An "element" or "node" is a nsIDOMNode.

nitty picky: an nsIDOMNode

::: toolkit/devtools/server/actors/profiler.js
@@ +171,5 @@
> +     * pass in a copy of them, not the originals.
> +     *
> +     * We break the cycle and make the copy by JSON.stringifying those values
> +     * with a replacer that omits properties known to introduce cycles, and then
> +     * JSON.parsing the result. This spends some CPU cycles, but it's simple.

There's something about this comment that is really rubbing me the wrong way, and I think it is because I feel like there are subtly incorrect assumptions about what the transport does and the nature of the protocol.

You don't send objects across the protocol, you send JSON packets. The transport's API is designed such that you give it non-stringified JSON data and it takes ownership of that data.

Because it is JSON data, that means there can't be cycles, can't be Infinity/NaN, etc.

Because the transport takes ownership of the objects you pass in, it is free to do its own optimizations and ensure safety depending on whether we are remote or local debugging. One such safety optimization for local debugging might be to copy the object, but it is actually just faster to deep freeze it. Again, this is fine because it owns the objects given to it.

I guess I just don't like the subtle implication that these things are the fault or limitation of the transport. It is doing exactly what it is designed to do, the behavior shouldn't be a surprise. Mistakenly interpreting the transport as an "object transport", makes it easier to make the mistake of trying to hold references to objects that have been sent across the transport, or to expect that mutations to an object sent along the transport will be visible on the client, or that sending Infinity/NaN across the protocol is ok (a mistake we've actually made!), etc. I know you aren't making this mistake here, but the tone of this comment encourages bad assumptions in others who might come later and who possibly don't understand the RDP/transport as well as you and I.

I think this comment should be rewritten to state something like:

"""
Create JSON objects suitable for transportation across the RDP by breaking cycles, etc...
"""

@@ +280,5 @@
> +  "registerEventNotifications": ProfilerActor.prototype.onRegisterEventNotifications,
> +  "unregisterEventNotifications": ProfilerActor.prototype.onUnregisterEventNotifications
> +};
> +
> +DebuggerServer.addGlobalActor(ProfilerActor, "profilerActor");

Can you use the newer, more modular register/unregister API? See http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/memory.js#93-101 and http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#403 for a good example.
Attachment #8453270 - Flags: review+
Attachment #8451745 - Flags: review?(nfitzgerald)
This is real nice. I tried to use it without anyone explaining the UI to me, and for the most part, everything is really intuitive, even the + to magnify works nicely. And I like the fact that I can highlight specific execution points in the detail view and see which parts in the graph it highlights. 

We should land this in nightly pref'd off so more people can play with it and provide feedback. 

What's missing: 
1) It would be nice to be able to expand the frame graph vertically so I can see more details on where time was spent.
2) I'll state the obvious, but it would be nice to have more in the timeline detail view than just the JS engine. It would be nice to be able to click on the various color segments in a slow frame and have it map to the details for that color segment. I figure this is planned, so ignore this if that is the case. 

If we do plan to provide the ability to view the details on other parts of the execution, then we will need to be able to expand the frame graph or at least have a way to hover&magnify to be able to select the colors and see different details. 

This is a great starting point and other people should try to use it and give more feedback.
> We should land this in nightly pref'd off so more people can play with it and provide feedback. 

While I usually agree we should control new features with prefs, in this case I disagree.

This UI has a superset of data presented in the existing UI, and the data that already is presented in the current UI is presented in pretty much the same way as it is in the existing UI, just prettier.

Plus, this UI actually has tests (in progress), while our fork of cleopatra doesn't.

There isn't any benefit to pref'ing this off and letting it cook.
I'm (as expected) very much with Nick on this one, there is negative value in landing this prefed off.
(In reply to Axel Kratel from comment #85)
> 
> What's missing: 
> 1) It would be nice to be able to expand the frame graph vertically so I can
> see more details on where time was spent.

I'll be working on bug 1007461 immediately after landing this.

> 2) I'll state the obvious, but it would be nice to have more in the timeline
> detail view than just the JS engine. It would be nice to be able to click on
> the various color segments in a slow frame and have it map to the details
> for that color segment. I figure this is planned, so ignore this if that is
> the case. 

Discussions are currently underway for building a timeline, which sounds close to what you're describing.

> This is a great starting point and other people should try to use it and
> give more feedback.

Thanks a bunch for the feedback!
Why did we rename this to Performance btw ? Profiler seems shorter and would look much better. We had to trim down Web Audio Editor to Web Audio for the tab to look better.
(In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #89)
> Why did we rename this to Performance btw ? Profiler seems shorter and would
> look much better. We had to trim down Web Audio Editor to Web Audio for the
> tab to look better.

Nick suggested it, because it's an evolving tool, and I felt like it made sense.
As I said above:

(Nick Fitzgerald [:fitzgen] from comment #26)
> [...] can we rename the tab from "Profiler" to "Performance"?
> This tool is evolving to be everything performance related, not just a JS
> profiler.

This is in regards to our plans to add platform events tracing and everything to the tool, so it isn't just a JS profiler anymore.
(In reply to Nick Fitzgerald [:fitzgen] from comment #84)

Thanks for the review! Agreed with everything, and will update the patch.

> ::: browser/devtools/profiler/profiler.js
> @@ +5,5 @@
> > +
> > +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> > +
> > +const require = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools.require;
> > +const promise = require("sdk/core/promise");
> 
> Shouldn't we be using Promise.jsm? Or do sdk promises wrap Promise.jsm now?
> 

Yes, and yes.

> Can this be lazily loaded with:
> 
>     devtools.lazyRequireGetter(this, "promise", "sdk/core/promise");

Probably.

> 
> @@ +13,5 @@
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> > +Cu.import("resource:///modules/devtools/SideMenuWidget.jsm");
> > +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> > +Cu.import("resource:///modules/devtools/Graphs.jsm");
> 
> Can we lazily load all of these? I see Task and (of course) XPCOMUtils is
> used immediately, but are all the rest as well?
> 

We can lazy load DevToolsUtils, SideMenuWidget and Graphs. ViewHelpers as well, if we were to pull Heritage from the sdk.

> 
> ::: browser/devtools/profiler/utils/shared.js
> @@ +38,5 @@
> > + * A connection to the profiler actor, along with other miscellaneous actors,
> > + * shared by all tools in a toolbox.
> > + *
> > + * Use `SharedProfilerConnection.forToolbox` to make sure you get the same
> > + * instance every time.
> 
> Should we just export the factory function, and not the constructor? Then we
> wouldn't need to tell people not to foot-gun, they just couldn't do it
> (unless they really tried to and used instance.constructor or
> Object.getConstructorOf or whatever its called but at that point people know
> they're doing something wrong and its not an accident anymore).
> 

This is already the case. ProfilerConnection is not exported.

> So, we would move `SharedProfilerConnection.forToolbox` to
> `exports.getProfilerConnection` or whatever.

Oh, so you mean only the method? Ok, I don't see why not!
Posted patch v11 (obsolete) — Splinter Review
Addressed Nick's comments.
Attachment #8444604 - Attachment is obsolete: true
Attachment #8451745 - Attachment is obsolete: true
Attachment #8453270 - Attachment is obsolete: true
Attachment #8455526 - Attachment is obsolete: true
Attachment #8451745 - Flags: review?(pbrosset)
Attachment #8456113 - Flags: review+
Posted patch v11 - reboot (rebased) (obsolete) — Splinter Review
Attachment #8456114 - Flags: review?(pbrosset)
Comment on attachment 8456114 [details] [diff] [review]
v11 - reboot (rebased)

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

Hard to find something to say after Rob's and Nick's reviews. I have made a few minor comments but overall this looks nice.
I have played with the profiler, with the patch applied, here are some things I noticed:

- Moving a selection sometimes fails: Draw a selection in the graphs, grab it with the mouse and start moving right or left. At some stage, the selection is not going to move anymore with the mouse.

- I got some weirdness using the zoom icon on a call row: Record a profile, click repeatedly on the zoom icon in one of the rows, quickly, until you get to only one item in the tree. What I noticed:
  - the graphs behave erratically, they get kind of blurred out for a while, or too big or too small, even if you keep clicking on the same line over and over again, the graph gets re-drawn
  - at some stage, the following error got thrown in the logs:
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: value is not a non-null object
Full stack: ProfileView.populateTab<@chrome://browser/content/devtools/ui-profile.js:145:1
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
ProfileView.addTabAndPopulate<@chrome://browser/content/devtools/ui-profile.js:183:11
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7

*************************

- I know it's not related to this bug because this was here before, but the tab approach is really weird. They look like tabs only they aren't tabs since they go away very easily (clicking on the root tab closes all the others). And there is definitely a relationship between because clicking on a previous tab closes all the next ones but not the ones before.
Without changing anything too major in this bug, maybe we could just re-style the tab so they looked like breadcrumbs? I think the metaphor would work a lot better.

- Still about tabs: do we really need the '+' icon? I really like the fact that the tree-view calls are filtered when you select a range, and I think this plus the zoom icon are enough, I don't really see this '+' icon used so much.

- The twisty expander icon in the tree-view doesn't always work correctly. STR: find an expanded parent row that has a collapsed child row. Dbl-click on the parent (it gets collapsed), dbl-click again (it expands) -> The child gets expanded too (why, btw?) but its expander icon remains in the collapsed position (once you get it this way, double-clicking on the child row only expands the icon, since the row itself is already expanded).

- Pressing the record button again to stop a recording should immediately show the "loading" label in the panel. There's a slight delay once you press it when nothing happens and it gives the impression that something is borken.

::: browser/devtools/profiler/panel.js
@@ +53,5 @@
> +  get target() this._toolbox.target,
> +
> +  destroy: Task.async(function*() {
> +    // Make sure this panel is not already destroyed.
> +    if (this._destroyed) {

We might want to also make sure that it's been opened first by adding a check for `this.isReady`

::: browser/devtools/profiler/profiler.js
@@ +72,5 @@
> +
> +/**
> + * Initializes the profiler controller and views.
> + */
> +let startupProfiler = Task.async(function*() {

It doesn't look like it's much of a problem because startupProfiler is only called once when the panel opens, but calling this function multiple times is probably going to lead to unexpected results.

::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
@@ +42,5 @@
> + *     node.appendChild(arrowNode).
> + *     ...
> + *     // Use `this.itemDataSrc` to customize the tree item and
> + *     // `this.level` to calculate the indentation.
> + *     node.MozMarginStart = (this.level * 10) + "px";

s/node.MozMarginStart/node.style.MozMarginStart

@@ +102,5 @@
> + *     ▶ lazily-added-node
> + *   ▼ bar
> + *     ▶ baz
> + *
> + * Everything else is taken care of automagically!

http://buukkit.appspot.com/img/search/magic.gif

::: toolkit/devtools/server/actors/profiler.js
@@ +6,5 @@
> +const {Cc, Ci, Cu, Cr} = require("chrome");
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");
> +
> +/**
> + * The profiler actor provides remote access to the built-in nsIProfiler module.

I think a short description of how to use this actor would be nice. Explaining the main methods and events, and the response data structure.

@@ +8,5 @@
> +
> +/**
> + * The profiler actor provides remote access to the built-in nsIProfiler module.
> + */
> +function ProfilerActor() {

Why not a protocol.js actor? What's the reasoning?

@@ +43,5 @@
> +   * that might have been accumulated so far.
> +   *
> +   * @param number entries
> +   * @param number interval
> +   * @param array:string features

These @params should be removed as the function doesn't actually accept any of them.
Attachment #8456114 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #96)
> Comment on attachment 8456114 [details] [diff] [review]
> v11 - reboot (rebased)
> 
> Review of attachment 8456114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks for the review!

> I have played with the profiler, with the patch applied, here are some
> things I noticed:
> 
> - Moving a selection sometimes fails: Draw a selection in the graphs, grab
> it with the mouse and start moving right or left. At some stage, the
> selection is not going to move anymore with the mouse.
> 

I can't reproduce this. Maybe you moved the mouse between graphs?

> - I got some weirdness using the zoom icon on a call row: Record a profile,
> click repeatedly on the zoom icon in one of the rows, quickly, until you get
> to only one item in the tree. What I noticed:
>   - the graphs behave erratically, they get kind of blurred out for a while,
> or too big or too small, even if you keep clicking on the same line over and
> over again, the graph gets re-drawn

Nice catch.

> - I know it's not related to this bug because this was here before, but the
> tab approach is really weird. They look like tabs only they aren't tabs
> since they go away very easily (clicking on the root tab closes all the
> others). And there is definitely a relationship between because clicking on
> a previous tab closes all the next ones but not the ones before.
> Without changing anything too major in this bug, maybe we could just
> re-style the tab so they looked like breadcrumbs? I think the metaphor would
> work a lot better.
> 

Yes, styling them as breadcrumbs will probably help a lot. My original plan was to simply add "x" (close) buttons on each tab, and make them actually behave like tabs. However, I couldn't get buttons to work inside <xul:tab> nodes, because any interaction, event listeners (and :hover pseudo-classes too) would simply be ignored by some css rule that I can't seem to overwrite.

> - Still about tabs: do we really need the '+' icon? I really like the fact
> that the tree-view calls are filtered when you select a range, and I think
> this plus the zoom icon are enough, I don't really see this '+' icon used so
> much.
> 

The "+" icon allows you to zoom in on the exact selection in the graph. Having a selection doesn't necessarily mean that the filtered tree's root node will span across the entire selection (so that you can use the loupe button). A selection filters the tree to show calls that *start* inside selection. Zooming in, on the other hand, clamps the samples.

> - The twisty expander icon in the tree-view doesn't always work correctly.
> STR: find an expanded parent row that has a collapsed child row. Dbl-click
> on the parent (it gets collapsed), dbl-click again (it expands) -> The child
> gets expanded too (why, btw?) but its expander icon remains in the collapsed
> position (once you get it this way, double-clicking on the child row only
> expands the icon, since the row itself is already expanded).
> 

Another nice catch!

> - Pressing the record button again to stop a recording should immediately
> show the "loading" label in the panel. There's a slight delay once you press
> it when nothing happens and it gives the impression that something is borken.
> 

This should already happen. Showing the 'Loading' message is the first thing that happens when selecting an item in the recordings list. The delay you're noticing is either because you have protocol logging enabled (and dumping all that data in stdout takes forever), or the browser is busy deserializing the protocol data (before even actually reaching the frontend). I'm not sure there's much I can do about it.

> 
> ::: toolkit/devtools/server/actors/profiler.js
> @@ +6,5 @@
> > +const {Cc, Ci, Cu, Cr} = require("chrome");
> > +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");
> > +
> > +/**
> > + * The profiler actor provides remote access to the built-in nsIProfiler module.
> 
> I think a short description of how to use this actor would be nice.
> Explaining the main methods and events, and the response data structure.
> 

Sure.

> @@ +8,5 @@
> > +
> > +/**
> > + * The profiler actor provides remote access to the built-in nsIProfiler module.
> > + */
> > +function ProfilerActor() {
> 
> Why not a protocol.js actor? What's the reasoning?
> 

Memory footprint on B2G.
Depends on: 1040390
Whiteboard: [c=devtools p= s= u=] → [c=devtools p= s= u=][status:inflight]
Attachment #8456113 - Attachment is obsolete: true
Attachment #8464933 - Flags: review+
Posted patch v12 - reboot (rebased) (obsolete) — Splinter Review
Attachment #8456114 - Attachment is obsolete: true
Attachment #8464934 - Flags: review?(rcampbell)
Posted patch v12 - tests (obsolete) — Splinter Review
I'm still not 110% happy with the coverage, but at this point I think I'm just stressing too much over nothing. This'll have to do for now, and we can address possibly untested code paths over the next few weeks.
Attachment #8464936 - Flags: review?(rcampbell)
Damn it, I had to rebase this again.
Attachment #8464933 - Attachment is obsolete: true
Attachment #8465448 - Flags: review+
Posted patch v13 - reboot (re-rebased) (obsolete) — Splinter Review
Attachment #8464934 - Attachment is obsolete: true
Attachment #8464934 - Flags: review?(rcampbell)
Attachment #8465450 - Flags: review?(rcampbell)
Posted patch v13 - tests (rebased) (obsolete) — Splinter Review
Attachment #8464936 - Attachment is obsolete: true
Attachment #8464936 - Flags: review?(rcampbell)
Attachment #8465451 - Flags: review?(rcampbell)
Comment on attachment 8465450 [details] [diff] [review]
v13 - reboot (re-rebased)

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

r+ shipit
Attachment #8465450 - Flags: review?(rcampbell) → review+
Comment on attachment 8465451 [details] [diff] [review]
v13 - tests (rebased)

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

yes.
Attachment #8465451 - Flags: review?(rcampbell) → review+
Try is giving me mixed messages: some leaks, and some failed win xp builds due to "something something recursive make backend". Pushing again, hoping all of that is unrelated.
Depends on: 1047124
Unfortunately I can't land this yet because of nsIProfile module leaks. I filed bug 1047124 in Core.
Looks like it might be a recent regression, since some of my older try runs didn't have any leaky oranges.
maybe related to the recent profiler changes around js deoptimization reporting?
Posted patch v14 (rebased)Splinter Review
Rebased, and fixed a small issue Rob found with odd/even rows not being colored properly.

I still can't land this until bug 1047124 is fixed. Again, technically, that bug isn't caused by this patch, but the fact that we've had the existing profiler tests disabled for so long allowed for that leak to stay latent.
Attachment #8465450 - Attachment is obsolete: true
Attachment #8467190 - Flags: review+
Attachment #8465451 - Attachment is obsolete: true
Attachment #8467191 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d6ea14edc3d4
https://hg.mozilla.org/mozilla-central/rev/f20b1f5b288d
https://hg.mozilla.org/mozilla-central/rev/d1adfecad34e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [c=devtools p= s= u=][status:inflight][fixed-in-fx-team] → [c=devtools p= s= u=][status:inflight]
Target Milestone: --- → Firefox 34
ZOMG.
Whiteboard: [c=devtools p= s= u=][status:inflight] → [c=devtools p= s= u=][status:landedon]
Release Note Request (optional, but appreciated)
[Why is this notable]: a better UI (web) people can work with
[Suggested wording]: Implemented new, improved UI of Profiler (Firefox Developer Tools).
[Links (documentation, blog post, etc)]: hacks.m.o?
relnote-firefox: --- → ?
Added in the 34 relnotes with wording "Improved User Interface of the Profiler"
> 
> +<!ENTITY profilerUI.recordButton.tooltip "Record JavaScript function calls."> 

Tootlips should never end with a period (also see bug 951132). Could anyone fix this in the future?
Depends on: 1136105
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.