879008 - New UI for the sampling Profiler

Status: RESOLVED FIXED

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

Description

[Anton Kovalyov (:anton)]

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.

Comment 1

[Benoit Girard (:BenWa)]

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.

Comment 2

[Anton Kovalyov (:anton)]

Attachment: nocleo.patch

WIP 1.

To see new histogram you will need to enable devtools.profiler.new-ui flag.

Comment 3

[Anton Kovalyov (:anton)]

Attachment: nocleo.patch

WIP 2 (UI stuff)

Comment 4

[Anton Kovalyov (:anton)]

Attachment: nocleo.patch

New minimap in the toolbar.

Comment 5

[Anton Kovalyov (:anton)]

Err forgot to add: WIP 3.

Comment 6

[Anton Kovalyov (:anton)]

Just applied this patch onto the current tree and everything looks good. Gonna finish it up and land before my vacation days.

Comment 7

[Anton Kovalyov (:anton)]

Attachment: WIP 4

Comment 8

[Anton Kovalyov (:anton)]

Attachment: WIP 5

Comment 9

[Anton Kovalyov (:anton)]

Attachment: WIP 6

Chart is ready more-or-less. Gonna work on the tree view next.

Comment 10

[Dave Huseby [:huseby]]

Tagging this so that the b2g perf team can track this work.

Comment 16

[Victor Porof [:vporof][:vp]]

Mockup: http://cl.ly/image/0O1T251O1U16

Comment 18

[Mathieu 'p01' Henri]

Is there flag in about:config for externals to test the new profiler ?

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

It's neither ready nor landed yet.

Comment 20

[Victor Porof [:vporof][:vp]]

Attachment: v1 (wip) - remove everything

Comment 21

[Victor Porof [:vporof][:vp]]

Attachment: v1 (wip) - reboot

Comment 22

[Victor Porof [:vporof][:vp]]

Attachment: screenshot (wip)

Comment 23

[Victor Porof [:vporof][:vp]]

Try builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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.

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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!

Comment 25

[Victor Porof [:vporof][:vp]]

Attachment: screenshot (v2 wip)

New screenshot showcasing filtering and a nicer tree view. Patches and builds on their way.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 27

[Victor Porof [:vporof][:vp]]

Attachment: v2 (wip) - remove everything

Comment 28

[Victor Porof [:vporof][:vp]]

Attachment: v2 (wip) - reboot

Comment 29

[Victor Porof [:vporof][:vp]]

Builds available soon at: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fd0950af7fb0/

Comment 30

[Victor Porof [:vporof][:vp]]

(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!

Comment 31

[James Long (:jlongster)]

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!

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> takes up quite a lot of vertical space

Maybe we can just shorten the height?

Comment 33

[Victor Porof [:vporof][:vp]]

(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.

Comment 34

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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

Comment 35

[Victor Porof [:vporof][:vp]]

(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.

Comment 36

[Victor Porof [:vporof][:vp]]

Attachment: remove everything

Comment 37

[Victor Porof [:vporof][:vp]]

Attachment: v3 (wip) - reboot

Comment 38

[Victor Porof [:vporof][:vp]]

New builds will be available soon at: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-82b0d37135c5/

Try: https://tbpl.mozilla.org/?tree=Try&rev=82b0d37135c5

Comment 40

[Victor Porof [:vporof][:vp]]

Attachment: v4 (reboot)

Cleaned up and documented everything.

Comment 41

[James Long (:jlongster)]

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.

Comment 42

[Victor Porof [:vporof][:vp]]

(In reply to James Long (:jlongster) from comment #41)

We'll have that in the flame graph view.

Comment 43

[James Long (:jlongster)]

(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.

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 45

[Victor Porof [:vporof][:vp]]

(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!

Comment 46

[Victor Porof [:vporof][:vp]]

New builds, get them while they're hot! https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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 

Comment 47

[Victor Porof [:vporof][:vp]]

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?

Comment 48

[James Long (:jlongster)]

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!

Comment 49

[Victor Porof [:vporof][:vp]]

(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/[e-mail]-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.

Comment 50

[James Long (:jlongster)]

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.

Comment 51

[Victor Porof [:vporof][:vp]]

Yup, I have! Consider that fixed ;)

Comment 52

[Victor Porof [:vporof][:vp]]

Attachment: screencap.gif

Clicking on legend items now makes a large-enough selection in the graph to contain all the bars containing the respective blocks.

Comment 53

[Victor Porof [:vporof][:vp]]

(not obvious from the gif, but that will also filter the call tree to match the selection bounds, just like with all selections)

Comment 54

[Tim Nguyen :ntim]

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.

Comment 55

[Victor Porof [:vporof][:vp]]

Yes, the graph canvases aren't theme-sensitive yet. I vote for handling that in a followup bug, if people are ok with it.

Comment 56

[Tim Nguyen :ntim]

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

Comment 57

[Victor Porof [:vporof][:vp]]

Thanks! I haven't tried it on Windows yet. Seems like we have a few style properties to add there.

Comment 58

[Victor Porof [:vporof][:vp]]

Wow, this really looks quite horrible on Linux and Windows!

Comment 59

[Victor Porof [:vporof][:vp]]

Attachment: v5 (reboot)

Safekeeping WIP patch with no test.

Comment 60

[Victor Porof [:vporof][:vp]]

Attachment: v6 (wip)

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.

Comment 61

[Victor Porof [:vporof][:vp]]

Attachment: v7 (wip)

Moved the AbstractTree implementation to its own file.

Comment 62

[Tim Nguyen :ntim]

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

Comment 63

[Victor Porof [:vporof][:vp]]

(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.

Comment 64

[Soledad Penades [:sole] [:spenades]]

I might have downloaded an older build from the link that Jeff sent us:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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!

Comment 65

[Victor Porof [:vporof][:vp]]

(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!

Comment 66

[Victor Porof [:vporof][:vp]]

Attachment: v8 (wip)

This should fix the UI issues on Windows and Linux.

Comment 67

[Victor Porof [:vporof][:vp]]

Attachment: v9

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/[e-mail]-55815405b8e4/

In the interest of landing this asap, I suggest reviewing this patch (modulo tests, which will follow sometimes this week).

Comment 68

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8444604 [details] [diff] [review]
remove everything

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

ship it.

Comment 69

[Rob Campbell [:rc] (:robcee)]

Reordering the flamechart dependency to blocked on this bug.

bug 1007461

Comment 70

[Victor Porof [:vporof][:vp]]

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/[e-mail]-5ad521cb8670/

Comment 71

[Benoit Girard (:BenWa)]

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.

Comment 72

[Victor Porof [:vporof][:vp]]

(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.

Comment 73

[Benoit Girard (:BenWa)]

(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.

Comment 74

[Rob Campbell [:rc] (:robcee)]

let's do those in some quick follow-ups. Already reviewing this patch and don't want it to change!

Comment 75

[Victor Porof [:vporof][:vp]]

(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.

Comment 76

[Benoit Girard (:BenWa)]

My feedback was not aimed at slowing down the landing in any way. Land away!

Comment 77

[Rob Campbell [:rc] (:robcee)]

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?

Comment 78

[Victor Porof [:vporof][:vp]]

(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.

Comment 79

[Rob Campbell [:rc] (:robcee)]

(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?

Comment 80

[Victor Porof [:vporof][:vp]]

(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.

Comment 81

[Victor Porof [:vporof][:vp]]

Attachment: v10

Addressed Rob's comments.

Comment 82

[Victor Porof [:vporof][:vp]]

Attachment: v10

Whoops, forgot the hg add the renamed tree.js

Comment 83

[Panos Astithas (he/him) [:past] (please ni?)]

(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 84

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 85

[Axel Kratel]

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.

Comment 86

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> 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.

Comment 87

[Victor Porof [:vporof][:vp]]

I'm (as expected) very much with Nick on this one, there is negative value in landing this prefed off.

Comment 88

[Victor Porof [:vporof][:vp]]

(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!

Comment 89

[Tim Nguyen :ntim]

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.

Comment 90

[Victor Porof [:vporof][:vp]]

(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.

Comment 91

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 92

[Victor Porof [:vporof][:vp]]

(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!

Comment 93

[Victor Porof [:vporof][:vp]]

Attachment: v11

Addressed Nick's comments.

Comment 94

[Victor Porof [:vporof][:vp]]

Attachment: v11 - remove everything (rebased)

Comment 95

[Victor Porof [:vporof][:vp]]

Attachment: v11 - reboot (rebased)

Comment 96

[Patrick Brosset <:pbro>]

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.

Comment 97

[Victor Porof [:vporof][:vp]]

(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.

Comment 98

[Victor Porof [:vporof][:vp]]

Attachment: v12 - remove everything (rebased)

Comment 99

[Victor Porof [:vporof][:vp]]

Attachment: v12 - reboot (rebased)

Comment 100

[Victor Porof [:vporof][:vp]]

Attachment: v12 - tests

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.

Comment 101

[Victor Porof [:vporof][:vp]]

Attachment: v13 - remove everything (re-rebased)

Damn it, I had to rebase this again.

Comment 102

[Victor Porof [:vporof][:vp]]

Attachment: v13 - reboot (re-rebased)

Comment 103

[Victor Porof [:vporof][:vp]]

Attachment: v13 - tests (rebased)

Comment 104

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8465450 [details] [diff] [review]
v13 - reboot (re-rebased)

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

r+ shipit

Comment 105

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8465451 [details] [diff] [review]
v13 - tests (rebased)

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

yes.

Comment 106

[Victor Porof [:vporof][:vp]]

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.

Comment 107

[Victor Porof [:vporof][:vp]]

Unfortunately I can't land this yet because of nsIProfile module leaks. I filed bug 1047124 in Core.

Comment 108

[Victor Porof [:vporof][:vp]]

Looks like it might be a recent regression, since some of my older try runs didn't have any leaky oranges.

Comment 109

[Rob Campbell [:rc] (:robcee)]

maybe related to the recent profiler changes around js deoptimization reporting?

Comment 110

[Victor Porof [:vporof][:vp]]

Attachment: v14 (rebased)

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.

Comment 111

[Victor Porof [:vporof][:vp]]

Attachment: v14 - tests (rebased)

Comment 112

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/d6ea14edc3d4
https://hg.mozilla.org/integration/fx-team/rev/f20b1f5b288d
https://hg.mozilla.org/integration/fx-team/rev/d1adfecad34e

Comment 113

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6ea14edc3d4
https://hg.mozilla.org/mozilla-central/rev/f20b1f5b288d
https://hg.mozilla.org/mozilla-central/rev/d1adfecad34e

Comment 114

[Victor Porof [:vporof][:vp]]

ZOMG.

Comment 115

[Florian Bender]

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?

Comment 116

[Sylvestre Ledru [:Sylvestre]]

Added in the 34 relnotes with wording "Improved User Interface of the Profiler"

Comment 117

[Ton [:Tonnes]]

> 
> +<!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?