Open Bug 1416714 Opened 7 years ago Updated 2 years ago

Netmonitor perceived performance enhancement

Categories

(DevTools :: Netmonitor, enhancement, P3)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: rickychien, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files)

Bug 1416714 - Remove domain column security lockbox icon
59 bytes, text/x-review-board-request
Details
Remove icons.png
240.69 KB, image/png
Details
Bug 1416714 - Remove cause column `JS` badge
59 bytes, text/x-review-board-request
Details
Bug 1416714 - Introduce border to request column & remove column alignment
59 bytes, text/x-review-board-request
Details
Bug 1416714 - Remove request list scroll to bottom
59 bytes, text/x-review-board-request
Details
Apply patch 1.png
251.69 KB, image/png
Details 
Here are some of observations from design and perceived perf aspect:

* Edge devtools network panel:
  text only, no thumbnail, no icon, no scroll to bottom

* Safari devtools network panel:
  text only, no thumbnail, no icon, no scroll to bottom

* Chrome devtools network panel:
  text, file thumbnail, no icon, scroll to bottom

* Firefox devtools network
  text, file thumbnail, status icon, domain lockbox icon, JS cause icon

We might want to rethink of removing scroll to bottom, because this is one of our major perf pains. After looking at mainstream browsers, Edge & Safari do not implement scroll to bottom but it's still handy for most cases.

In order to reduce noticeable visual updates, I'd propose to get rid of these dynamic displayed icons. User might feel slow when watching those colorful icons rendered on the table dynamically. Comparing with plain text, icon change is more noticeable by eyes, so making user notices that our tabular data is updated slowly. Removing those icons could get a lots perceived performance improvement IMO.

This idea makes perceived perf improvement, but it might not have impact on damp test.

I'm going to start working on a prototype to remove scroll to bottom, dynamic icons, file thumbnails and try my best to make the visible requests to be updated more quickly.
> This idea makes perceived perf improvement, but it might not have impact on
> damp test.

Anything that slow down computation should have an impact on DAMP, especially the two requestsFinished tests.
Using perf-html works great with DAMP as anything that appears on it means it is slowing completiong of the profiled scenario. And so it would also slow down DAMP computation and result.

ScrollToBottom should be a good example, it appears a lot in perf-html and I'm expecting it to have significant impact on DAMP results. If not DAMP tests should be reviewed.
Attached image Remove icons.png — 
Victoria,

Do you have any opinion about this UI proposal? In order to gain perf improvement, we should really think of removing some features like scroll to bottom. Using some tricks to improve perceived perf. What do you think?
Flags: needinfo?(victoria)
(In reply to Ricky Chien [:rickychien] from comment #3)
> DAMP result indicates high confidence for complicated cases.
> 
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=mozilla-
> central&newProject=try&newRevision=610ca7e61de417361cb630b418148c6b9f355fb4&o
> riginalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec6
> 6500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTime
> Range=172800

15% is significant, but you are suggesting to strip many features all at once.

It would be interesting to know what is the cost of them individually.
For example it isn't clear what is the cost of icons versus scrollToBottom,
which are very different things.
Yep, good idea. I'm going to remove scrollToBottom and see damp result.
DAMP result of removing scrollToBottom

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=39386c974633e2745479772fe3257ef11bb1dfcf&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800


It's really interesting, it shows -3.03% with high confidence in complicated.netmonitor.requestsFinished.DAMP and still looks good. But compare with the result of comment 3, it tells that strip other features is still effective.
I especially like the idea of removing the status icons since we could just color the text of the status codes -- 400 would be colored Green 70, 300 could be colored Yellow 70, etc. Use a gray — for none.

Would you still be able to hover over images to see the preview? In that case we could use an ASCII character or something to denote "hover over me to see preview."

Could you explain what "scroll to bottom" means here? :) Don't think I've used/seen that.
Flags: needinfo?(victoria)
When monitoring network request in netmonitor table, table scrollbar will always scroll to bottom to make sure user can see the latest network request. You can just open netmonitor and then visit a popular website like http://cnn.com to see how requests table always scroll to bottom.

I don't think this is a necessary behavior for web developer. While I try debugging in netmonitor, I don't have strong expectation to see the latest request. There are plenty of aspects for developers to find out network issues, like focusing on waterfall timeline to see which requests are slow, or look through all requests to ensure that all requests are loaded properly...etc.

Removing scrollToBottom is able to reduce -3.03% (see comment 7), so it might be really effective for perf improvement.
(In reply to Victoria Wang [:victoria] from comment #8)
> Would you still be able to hover over images to see the preview? In that
> case we could use an ASCII character or something to denote "hover over me
> to see preview."

Image thumbnail will be removed in bug 1404917 in order to avoid loading large amount of content data and address perf issue. The patch of bug 1404917 is going to be landed soon. As a result, my patch is just a proof of concept to remove image thumbnail UI to see what it looks like.
Strip status icon, domain lockbox icon, cause "JS". (enable image thumbnail, scroll to bottom)


damp complicated.netmonitor.close.DAMP 142.30 ± 2.58% > 132.57 ± 3.56%	-6.84%	4.20 (med)

damp complicated.netmonitor.requestsFinished.DAMP 9,679.93 ± 0.76% > 9,510.56 ± 0.38%	-1.75%	6.79 (high)


https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=5e1f0743905b&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

It turns out that strip out those icons does affect to perf.
(In reply to Victoria Wang [:victoria] from comment #8)
> I especially like the idea of removing the status icons since we could just
> color the text of the status codes -- 400 would be colored Green 70, 300
> could be colored Yellow 70, etc. Use a gray — for none.

Be careful with accessibility... data should never be conveyed by color only. Here we'd have both the status code and the color, but I believe the icon brings something more. For something so static I believe we can keep it and still be performant.

> 
> Would you still be able to hover over images to see the preview? In that
> case we could use an ASCII character or something to denote "hover over me
> to see preview."

I'd like to understand what's slow here; especially I think that displaying thumbnails of big images is not useful because they're not recognizable, yet thumbnails of small images can be. Although I wouldn't miss this so much.
In comment 3, DAMP reported a 15% win on complicated.requestsFinished, which is a very significative improvement.
In this patch you disabled many things including scrollToBottom and various UI elements.

In comment 7, you only disabled scrollToBottom and got "only" a 3% win on the same DAMP test. (3% is still quite significant for this test!)

In comment 11, it looks like you took back patch from comment 3, but re-enabled scrollToBottom. Is that it?
If that's the case, this is disturbing as it now only report 1.75% win. Where did the 15%-3%-1.75=10.25% went?

(By the way, the various "close" test are not so relevant, they have a very high variance and I'm not confident about them)
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> In comment 11, it looks like you took back patch from comment 3, but
> re-enabled scrollToBottom. Is that it?

You're right.

> If that's the case, this is disturbing as it now only report 1.75% win.
> Where did the 15%-3%-1.75=10.25% went?

I suspect that 10.25% might be due to image thumbnail in file column. Let wait and see what damp results say.

There several damp tests are ongoing. I striped each feature piece by piece from status icon, domain lockbox icon, cause column JS icon to file column image thumbnail individually. Hopefully we can identify which one is the perf bottleneck.

Finally, back to the description. My proposal is not aiming at damp perf, but perceived perf (user feeling).
About scrolling to bottom, I'd like to give some ideas, coming from how we implemented the same thing in Firefox OS.

1. We need to know if we're at the bottom.

For this, we used the "scroll" event and ran this code:

    var scrollTop = this.container.scrollTop;
    var scrollHeight = this.container.scrollHeight;
    var clientHeight = this.container.clientHeight;

    this.isScrolledManually = ((scrollTop + clientHeight) < scrollHeight);

This _will_ trigger a synchronous reflow if a new object is added while scrolling. But because scrolling is asynchronous these days, this shouldn't be noticeable for the user, yet this can bring perf issue from the fact we're in the main thread.

But this brings the nice thing that we now have a very cheap boolean to check when we need it, instead of calling getBoundingClientRect each time we render.

2. How do we scroll to bottom ?

We simply used this:

      this.container.lastElementChild.scrollIntoView(false);

and let the browser do its work, possibly asynchronously (I don't know :) ).

When I compare with the current code in the netmonitor:

    // Keep the list scrolled to bottom if a new row was added
    if (this.shouldScrollBottom && node.scrollTop !== MAX_SCROLL_HEIGHT) {
      // Using maximum scroll height rather than node.scrollHeight to avoid sync reflow.
      node.scrollTop = MAX_SCROLL_HEIGHT;
    }

What strikes me is that we access `scrollTop` in the condition, which _does_ trigger a sync reflow. Using `scrollIntoView` may prevent this.

So here are my 2 cents. I'm not saying this is the best thing to do this, and maybe as you said we could just remove it.
(In reply to Julien Wajsberg [:julienw] from comment #15)
> What strikes me is that we access `scrollTop` in the condition, which _does_
> trigger a sync reflow. Using `scrollIntoView` may prevent this.

https://gist.github.com/paulirish/5d52fb081b3570c81e3a#scroll-stuff
Looks like it is on the list of things forcing a reflow...

(In reply to Ricky Chien [:rickychien] from comment #14)
> There several damp tests are ongoing. I striped each feature piece by piece
> from status icon, domain lockbox icon, cause column JS icon to file column
> image thumbnail individually. Hopefully we can identify which one is the
> perf bottleneck.

Cool, thanks for doing that!

> Finally, back to the description. My proposal is not aiming at damp perf,
> but perceived perf (user feeling).

DAMP results translate into what users experience.
Now, it is not true for all of its tests, some are still flaky like "close" ones.
But "requestsFinished" is a good one, you can trust.
Wins on this particular test clearly translate into visible wins for the user!

You should be very careful about "perceived performance". Unless the win is really big,
you could really easily fall into a false perception. For example, it is hard to reproduce
twice the same performance on Firefox due to background task running on your OS and in Firefox itself.
So when you run an experiment manually, it is always hard to know what you really see.
I'm going to focus on complicated.netmonitor.requestsFinished.DAMP case

Remove status icon: 
delta: -0.25%
confidence: low

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=1dab7ea06636&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800


Remove domain lockbox icon:
delta: -2.54%
confidence: high

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=03d65583b195&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800


Remove cause column JS icon:
delta: -1.36%
confidence: medium

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=7643b36b618e&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800


Remove file column image thumbnail
delta: -12.23%
confidence: high

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=cd390797a480&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800
(In reply to Alexandre Poirot [:ochameau] from comment #16)
> (In reply to Julien Wajsberg [:julienw] from comment #15)
> > What strikes me is that we access `scrollTop` in the condition, which _does_
> > trigger a sync reflow. Using `scrollIntoView` may prevent this.
> 
> https://gist.github.com/paulirish/5d52fb081b3570c81e3a#scroll-stuff
> Looks like it is on the list of things forcing a reflow...

Yes, as does setting to scrollTop. But it's not synchronous as far as I know (but we should verify this claim).
But _getting_ scrollTop triggers a synchronous reflow. That's my point here.
According to above outcome, here are my analysis:

Remove domain lockbox icon

diff: https://hg.mozilla.org/try/rev/03d65583b195aca07f6e014db4da1829352c99b2

Confidence is high because we can avoid painting a lockbox icon as well as using L10N.getStr() API.


Remove cause column JS icon:

diff: https://hg.mozilla.org/try/rev/7643b36b618e8aefb610f43c64e4a2f839cb3aa9

Confidence is medium because there is an extra DOM operation `causeHasStack && div({ ... })`.


Remove file column image thumbnail:

diff: https://hg.mozilla.org/try/rev/cd390797a4804d5dcfd40b853157f9773e7d4992

Confidence is high because we know that processing large image dataURI into smaller thumbnail will consume lots of CPU / GPU resources.
Summary: Netmonitor perceived performance experiment → Netmonitor perceived performance enhancement
(In reply to Alexandre Poirot [:ochameau] from comment #16)
> You should be very careful about "perceived performance". Unless the win is
> really big,
> you could really easily fall into a false perception. For example, it is
> hard to reproduce
> twice the same performance on Firefox due to background task running on your
> OS and in Firefox itself.

If we do not always scroll to bottom while loading, users are able to see completed results earlier for those first-come network requests. If we always enable scroll to bottom, user might see plenty of columns are empty or stay in initial state since those followup `network event update` packets have arrived yet.
s/have arrived yet/havn't arrived yet/
Attachment #8927777 - Attachment is obsolete: true
Attached image Apply patch 1.png — 
Uploaded patch for enhancing perceived perf based on damp result, please see also comment 17.

Because removing status icon doesn't indicate much perf improvement, current patch isn't going to remove it until we reach consensus.

According to previous damp result, we can expect that at least -2.54% (domain) + -1.36% (cause) + -3.03% (scroll to bottom) = -6.94% for complicated.netmonitor.requestsFinished.DAMP case.

Considering table typography and column alignment after removing "JS" badge and lock-box icon, I removed column's text-align: center and introduced borders between each column for readability (see attachment).
Victoria & Harald,

Do you have any opinion on this change (see comment 27)? Do you feel okay if we remove scroll to bottom (see comment 9)?

In order to help address perf issue, we should make trade-off for some kind of reasons. On the other hand, it's also a good chance for us to think about redesign. Some column might not be very important as being default columns, like cause column could be hidden by default. Some information is not that useful such as lock icon in domain column, "JS" icon in cause column. In fact, both icons are clickable, but it's barely noticeable by normal user.
Flags: needinfo?(victoria)
Flags: needinfo?(hkirschner)
(In reply to Ricky Chien [:rickychien] from comment #29)
> In order to help address perf issue, we should make trade-off for some kind
> of reasons.

Just to be clear here. The usage of "should" is wrong here.
I would rather phrase it like this:
  "We can improve performance with trade-off where we disable some UI element or change UI behavior."

Also note that, before having to strip features, we can still continue improving performance without any trade-off.
See bug 1404929 with 4 to 13% win and bug 1404913 with two distinct wins with 8 to 24%.

> On the other hand, it's also a good chance for us to think about
> redesign.

Having said that, this is a good idea.
Especially if Victoria has enough bandwidth to really go over the netmonitor.
I mostly wanted to make it clear that it isn't the only way to improve netmonitor perfs.
I’d like to better understand the bottlenecks that this removes. Perceived performance is one thing, but this is mostly stripping out features. Could you share perf profiles for a before/after comparison?
Flags: needinfo?(hkirschner)
After discussion at today's meeting we agreed on:
* Focus on lazy loading data.
* Try Julien's suggestion in comment #15 (async scroll-to-bottom)

We might want to get back to this bug and to some UI changes as soon as other more promising options (like lazy loading and removing Immutable JS) are done.

Honza
Attachment #8927777 - Flags: review?(odvarko)
Attachment #8928409 - Flags: review?(odvarko)
Attachment #8928410 - Flags: review?(odvarko)
Attachment #8928411 - Flags: review?(odvarko)
Leave this issue open because this would be a long term plan to discuss how to redesign the request list table. Wait for Victoria's feedback.
Assignee: rchien → nobody
Status: ASSIGNED → NEW
(In reply to Julien Wajsberg [:julienw] from comment #18)
> Yes, as does setting to scrollTop. But it's not synchronous as far as I know
> (but we should verify this claim).
> But _getting_ scrollTop triggers a synchronous reflow. That's my point here.

About that, note that we are running in chrome/parent process and we don't have APZ.
It may be a different setup compared to FxOS.
@Alex: yep. Still getting scrollTop guarantees you a synchronous reflow, while the other options don't. It doesn't mean they're OK. :)
Flags: needinfo?(victoria)
(In reply to Jan Honza Odvarko [:Honza] from comment #36)
> * Try Julien's suggestion in comment #15 (async scroll-to-bottom)

Opened bug 1429824 to focus on scroll to bottom.
See Also: → 1429824
Product: Firefox → DevTools
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: