946601 - Add performance statistics/piecharts in the Network Monitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Comment 1

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

Attachment: screenshot.png

Anything you feel should look differently here, Darrin? This was somewhat an inspiration: http://www.epiphanysearch.co.uk/blog/wp-content/uploads/2011/09/YSlow-Before-Stats.png and the final functionality should resemble that.

Comment 2

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

Does it make sense to show not just size of downloads per type, but time spent downloading per type?

Comment 3

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> Does it make sense to show not just size of downloads per type, but time
> spent downloading per type?

I think that's a good thing to show, definitely. I'll add it.

Comment 4

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

Attachment: screencast.gif

Added total time spent per request type.

This is how the piechart feels like when interacting with it.

Clicking on a slice will take you back to the network "table mode" and filter by that request type.

Comment 5

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

Attachment: v1

Work in progress...

Comment 6

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

Attachment: v2

Cleanup.

Comment 7

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

Attachment: updated screenshot

Comment 8

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

Looks pretty :)

Comment 9

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

Attachment: v3

Alright, I'm happy with this implementation. Needs some more tests, but should be ready for some feedback.

Rob, can you please review this while I finish with the tests, in the interest of landing this before the merge.

Comment 10

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

Comment on attachment 8361223 [details] [diff] [review]
v3

Nope, this isn't working properly :|
Take 2 :)

Comment 11

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

Attachment: v4

Comment 12

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

Attachment: v5

Alrighty, this is better :) Rob, see comment 9.

Comment 13

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

Couple of quick observations: debug protocol traffic is pretty heavy when switching to this view. Are we ferrying all information over the protocol every time we make the switch?

I'd like to get some UI feedback on the design and interaction. I think it's pretty good, personally, but would like an expert's opinion.

Could we get a try build linked in here for Darrin to play with?

Comment 14

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #13)
> Couple of quick observations: debug protocol traffic is pretty heavy when
> switching to this view. Are we ferrying all information over the protocol
> every time we make the switch?
> 

It's reloading the page, so all traffic is just like normal traffic when performing a manual reload.

I don't think it's a good idea to optimize and compute all of this on the server-side, since the transferred data is still shown in the network waterfall view when switching away from the statistics view. You wouldn't want that table to be empty after going back to the waterfall view, would you? :)

Comment 15

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

Pushed to try, builds will be available soon.

Comment 16

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

Here be built builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c4ae7761e281/

Comment 17

[Darrin Henein [:darrin]]

Comment on attachment 8361563 [details] [diff] [review]
v5

Looks pretty good! Couple of questions/concerns:

1. When you hover a slice of the pie graph, the highlighted row in the table is very subtle. Rather than the dotted outline, can we fade the remaining rows leaving the 'selected' row brighter? I think this would better communicate the intended correlation.

2. I don't like the back button/interaction. Not sure what else to do... could this UI work as a panel/doorhanger off of the requests link (that currently triggers the view change?) Thoughts?

3. The feature is not very discoverable. Without watching your screencast it took me a minute or two to find where the entry point was. Maybe we need a better icon/affordance in the initial link (vs. the speedometer icon we have now)?

Comment 18

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

(In reply to Darrin Henein [:darrin] from comment #17)
> Comment on attachment 8361563 [details] [diff] [review]
> v5
> 
> Looks pretty good! Couple of questions/concerns:
> 

Thank you!

> 1. When you hover a slice of the pie graph, the highlighted row in the table
> is very subtle. Rather than the dotted outline, can we fade the remaining
> rows leaving the 'selected' row brighter? I think this would better
> communicate the intended correlation.
> 

Sure!

> 2. I don't like the back button/interaction. Not sure what else to do...
> could this UI work as a panel/doorhanger off of the requests link (that
> currently triggers the view change?) Thoughts?

I don't think a panel is a good idea. This tool causes a target navigation and rebuilds the entire network requests table, so there's going to be too much stuff going on behind that supposedly transient panel. A different view is the best approach imho, but much better thing to think about is 3.

> 3. The feature is not very discoverable. Without watching your screencast it
> took me a minute or two to find where the entry point was. Maybe we need a
> better icon/affordance in the initial link (vs. the speedometer icon we have
> now)?

Agreed, apart from the initial button which is shown when the netmonitor is empty, the other points of entry are a bit camouflaged. I don't have any good idea, better icons would be the best bet maybe?

Comment 19

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

Comment on attachment 8361563 [details] [diff] [review]
v5

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

a couple of typos and a bug fix below.

canceling review.

::: browser/devtools/netmonitor/netmonitor-controller.js
@@ +80,5 @@
> +    WITH_CACHE_ENABLED: 1,
> +    WITH_CACHE_DISABLED: 2
> +  },
> +
> +  // Enabling or disabling the cache without trigerring a reload.

typo: triggering.

@@ +313,5 @@
> +      });
> +      return deferred.promise;
> +    };
> +
> +    // Reconfigures the tab, optionally trigerring a reload.

triggggerring

@@ +316,5 @@
> +
> +    // Reconfigures the tab, optionally trigerring a reload.
> +    let reconfigureTab = aOptions => {
> +      let deferred = promise.defer();
> +      this.client.reconfigureTab(aOptions, deferred.resolve);

this line is throwing an error. Did we miss something?

A coding exception was thrown and uncaught in a Task.

Full message: TypeError: this.client.reconfigureTab is not a function
Full stack: NetMonitorController.triggerActivity/reconfigureTab@chrome://browser/content/devtools/netmonitor-controller.js:320

this should now be

this._target.activeTab.reconfigure(...

thanks past!

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +57,5 @@
>    preventDescriptorModifiers: true,
>    eval: () => {},
>    switch: () => {}
>  };
> +const NETWORK_ANALYSIS_PIE_CHART_DIAMETER = 200; // px

I think I'd like to see a minimum diameter and some ability to scale a bit. More after I paste this.

Comment 20

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

Attachment: beachball.json.zip

on viewing some large sites (I picked CNN and learned all kinds of things about Justin Bieber!) switching to the chart view causes a sizeable beachball.

Most of the time was spent in netmonitor-view.js::_flushWaterfallViews (line 1074). Surprising considering I'm not viewing the waterfall.

Comment 21

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

A couple of interface issues:

Resizing the toolbox while the chart is visible is quite slow. Not sure if it's the mediaquery or some other recalculation, but it's kind of sluggish.

There's some clipping in the little stats display at the bottom of the waterfall. (see attach)

Not sure what the minimum width of the chart is but it feels wide. Below a certain threshold the graph flips to vertical orientation. This still clips a bunch of the table requiring a pretty wide toolbox. The usable minimum width in either orientation is wide. Not sure what I'd recommend here. Maybe Darrin can provide some feedback.

Comment 22

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

Attachment: clipped-stats.png

Comment 23

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #20)
> Created attachment 8365144 [details]
> beachball.json.zip
> 
> on viewing some large sites (I picked CNN and learned all kinds of things
> about Justin Bieber!) switching to the chart view causes a sizeable
> beachball.
> 
> Most of the time was spent in netmonitor-view.js::_flushWaterfallViews (line
> 1074). Surprising considering I'm not viewing the waterfall.

Most likely bug 956452

Comment 24

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #23)

> > Most of the time was spent in netmonitor-view.js::_flushWaterfallViews (line
> > 1074). Surprising considering I'm not viewing the waterfall.
> 
> Most likely bug 956452

yeah, could be. Not sure we should be flushing the waterfall at all when we're showing the network stats.

Another thought, Victor: We should probably have a kill switch for this if we need to do some perf tuning with it checked in.

Comment 25

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #24)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #23)
> 
> > > Most of the time was spent in netmonitor-view.js::_flushWaterfallViews (line
> > > 1074). Surprising considering I'm not viewing the waterfall.
> > 
> > Most likely bug 956452
> 
> yeah, could be. Not sure we should be flushing the waterfall at all when
> we're showing the network stats.
> 

Definitely some room for optimization in the context of this bug. I'll work on it.

> Another thought, Victor: We should probably have a kill switch for this if
> we need to do some perf tuning with it checked in.

Good idea.

Comment 26

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #21)
> A couple of interface issues:
> 
> Resizing the toolbox while the chart is visible is quite slow. Not sure if
> it's the mediaquery or some other recalculation, but it's kind of sluggish.
> 

It's because of the network requests table, which is not visible. You'd think XUL would be smart about it and not draw it when it's in a deck, not visible nor affecting layout :) I'll manually display:none and see if it makes any difference.

Comment 27

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

Attachment: net-pie.patch

A'ight. Rebased on top of 957117, themed the new statistics view, made those performance optimizations, addressed comments and ui feedback, etc.

This was somewhat fun.

I'll attach the tests shortly.

Comment 28

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

Attachment: v7

'tis done.

Comment 29

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

Comment on attachment 8368626 [details] [diff] [review]
v7

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

alright, looks great!

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1365,5 @@
>    /**
>     * The resize listener for this container's window.
>     */
>    _onResize: function(e) {
> +    // Don't paint things while the waterfall view isn't even visible.

thaaanks!

Comment 30

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

Zomg. Thanks!

Comment 31

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

https://hg.mozilla.org/integration/fx-team/rev/b3a41a5f1972

Comment 32

[David Bruant]

I LOVE THIS!

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b3a41a5f1972

Comment 34

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

(In reply to David Bruant from comment #32)
> I LOVE THIS!

DEVELOPERS DEVELOPERS DEVELOPERS!

Comment 35

[u60234]

Comment on attachment 8368626 [details] [diff] [review]
v7

>diff --git a/browser/locales/en-US/chrome/browser/devtools/netmonitor.properties b/browser/locales/en-US/chrome/browser/devtools/netmonitor.properties
 
> # LOCALIZATION NOTE (networkMenu.minute): This is the label displayed
> # in the network menu specifying timing interval divisions (in minutes).
> networkMenu.minute=%S min
>+
>+# LOCALIZATION NOTE (networkMenu.minute): This is the label displayed
>+# in the network menu specifying timing interval divisions (in minutes).
>+networkMenu.minute=%S min
>+

This patch duplicated the networkMenu.minute string.

Comment 36

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

Whoops! We're going to fix it shortly!

Comment 37

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

Filed bug 966789.

Comment 38

[Francesco Lodolo [:flod]]

<!ENTITY netmonitorUI.footer.perf   "Toggle performance analysis...">
<!ENTITY netmonitorUI.context.perfTools   "Start Performance Analysis...">

You should use a single unicode character here (…), not "...". That's the standard for en-US and can be fixed without new IDs for the strings.

One more doubt about "Primed cache". Can you explain what exactly this label describes?

Comment 39

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

"Primed cache" is means cache has been filled and loaded from.
I filed bug 966894 for the ellipsis.

Comment 40

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

The original patch had an "Other" button in the netmonitor's footer, but it got lost during a rebase somehow. So I landed it separately now: https://hg.mozilla.org/integration/fx-team/rev/0889de4ea718

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0889de4ea718

Comment 42

[Will Bamberg [:wbamberg]]

I've added some documentation for this: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Performance_analysis. Please let me know if I've missed important stuff.

Comment 43

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

(In reply to Will Bamberg [:wbamberg] from comment #42)
> I've added some documentation for this:
> https://developer.mozilla.org/en-US/docs/Tools/
> Network_Monitor#Performance_analysis. Please let me know if I've missed
> important stuff.

Love it Will, thank you! As a general comment, you might want to consider creating new screenshots for the timeline, since bug 909251 landed and it changes the waterfall appearance even more!

Comment 44

[Alfred Kayser]

requests-menu-network-summary-button and requests-menu-network-summary-label do the same thing and the label could be combined into the button itself.