Add performance statistics/piecharts in the Network Monitor

RESOLVED FIXED in Firefox 29

Status

P2
normal
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 29
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Comment 1

5 years ago
Created attachment 8359281 [details]
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.
Attachment #8359281 - Flags: ui-review?(dhenein)
Does it make sense to show not just size of downloads per type, but time spent downloading per type?
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 8359714 [details]
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.
(Assignee)

Comment 5

5 years ago
Created attachment 8359800 [details] [diff] [review]
v1

Work in progress...
(Assignee)

Comment 6

5 years ago
Created attachment 8360245 [details] [diff] [review]
v2

Cleanup.
Attachment #8359800 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 8361158 [details]
updated screenshot
Attachment #8359281 - Attachment is obsolete: true
Attachment #8359281 - Flags: ui-review?(dhenein)
Looks pretty :)
(Assignee)

Comment 9

5 years ago
Created attachment 8361223 [details] [diff] [review]
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.
Attachment #8360245 - Attachment is obsolete: true
Attachment #8361223 - Flags: feedback?(rcampbell)
(Assignee)

Comment 10

5 years ago
Comment on attachment 8361223 [details] [diff] [review]
v3

Nope, this isn't working properly :|
Take 2 :)
Attachment #8361223 - Attachment is obsolete: true
Attachment #8361223 - Flags: feedback?(rcampbell)
(Assignee)

Comment 12

5 years ago
Created attachment 8361563 [details] [diff] [review]
v5

Alrighty, this is better :) Rob, see comment 9.
Attachment #8361533 - Attachment is obsolete: true
Attachment #8361563 - Flags: review?(rcampbell)
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?
Attachment #8361563 - Flags: ui-review?(dhenein)
(Assignee)

Comment 14

5 years ago
(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? :)
(Assignee)

Comment 15

5 years ago
Pushed to try, builds will be available soon.
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)?
(Assignee)

Comment 18

5 years ago
(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 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.
Attachment #8361563 - Flags: ui-review?(dhenein)
Attachment #8361563 - Flags: review?(rcampbell)
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.
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.
Created attachment 8365146 [details]
clipped-stats.png
(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
(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.
(Assignee)

Comment 25

5 years ago
(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.
(Assignee)

Comment 26

5 years ago
(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.
(Assignee)

Comment 27

5 years ago
Created attachment 8367829 [details] [diff] [review]
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.
Attachment #8359714 - Attachment is obsolete: true
Attachment #8361158 - Attachment is obsolete: true
Attachment #8361563 - Attachment is obsolete: true
Attachment #8365144 - Attachment is obsolete: true
Attachment #8365146 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 8368626 [details] [diff] [review]
v7

'tis done.
Attachment #8367829 - Attachment is obsolete: true
Attachment #8368626 - Flags: review?(rcampbell)
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!
Attachment #8368626 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 30

5 years ago
Zomg. Thanks!

Comment 32

5 years ago
I LOVE THIS!
https://hg.mozilla.org/mozilla-central/rev/b3a41a5f1972
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
(Assignee)

Comment 34

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

DEVELOPERS DEVELOPERS DEVELOPERS!

Comment 35

5 years ago
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.
(Assignee)

Comment 36

5 years ago
Whoops! We're going to fix it shortly!
(Assignee)

Comment 37

5 years ago
Filed bug 966789.
<!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?
(Assignee)

Comment 39

5 years ago
"Primed cache" is means cache has been filled and loaded from.
I filed bug 966894 for the ellipsis.
Depends on: 967378
(Assignee)

Comment 40

5 years ago
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
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.
Flags: needinfo?(vporof)
(Assignee)

Comment 43

5 years ago
(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!
Flags: needinfo?(vporof)

Comment 44

5 years ago
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.

Updated

5 years ago
Blocks: 969793

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.