Closed Bug 946601 Opened 7 years ago Closed 6 years ago

Add performance statistics/piecharts in the Network Monitor

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 11 obsolete files)

No description provided.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P2
Attached image screenshot.png (obsolete) —
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?
(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.
Attached image screencast.gif (obsolete) —
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.
Attached patch v1 (obsolete) — Splinter Review
Work in progress...
Attached patch v2 (obsolete) — Splinter Review
Cleanup.
Attachment #8359800 - Attachment is obsolete: true
Attached image updated screenshot (obsolete) —
Attachment #8359281 - Attachment is obsolete: true
Attachment #8359281 - Flags: ui-review?(dhenein)
Attached patch v3 (obsolete) — Splinter Review
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)
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)
Attached patch v4 (obsolete) — Splinter Review
Attached patch v5 (obsolete) — Splinter Review
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)
(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? :)
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)?
(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)
Attached file beachball.json.zip (obsolete) —
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.
Attached image clipped-stats.png (obsolete) —
(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.
(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.
(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.
Attached patch net-pie.patch (obsolete) — Splinter Review
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
Attached patch v7Splinter Review
'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+
Zomg. Thanks!
I LOVE THIS!
https://hg.mozilla.org/mozilla-central/rev/b3a41a5f1972
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
(In reply to David Bruant from comment #32)
> I LOVE THIS!

DEVELOPERS DEVELOPERS DEVELOPERS!
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.
Whoops! We're going to fix it shortly!
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?
"Primed cache" is means cache has been filled and loaded from.
I filed bug 966894 for the ellipsis.
Depends on: 967378
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)
(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)
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.
Blocks: 969793
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.