Open Bug 1421926 Opened 5 years ago Updated 4 years ago

Show one of content size or transferred size by default to reduce the requestsFinished time

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: gasolin, Unassigned)

References

Details

Attachments

(1 file)

Harald, this sounds like significant perf improvement and having two columns related to size displayed by default doesn't sound like something we have to have.

Would you agree with having just "Size" column visible
by default and "Transferred" hidden?

Honza
Flags: needinfo?(hkirschner)
Nice investigation!

The win for just one removed column is really big.
It would be interesting to know why this is so big.
There might be lessons to learn in order to optimize the columns we still display!

If you look at contentSize column component, this is doing almost nothing.
So it could be useful to deconstruct this component piece by piece to see from where comes such win.
For example:
* stop calling getFormatedSize, to see if it has any impact
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListColumnContentSize.js#27
I'm wondering what is the cost of getFormatedSize, julian optimized that I think, but it may still be unnecessary slow.
* make shouldComponentUpdate return false, to see if it tries to update too frequently
* make render return nothing, but still compute what it currently does, to see if that's slow just because of the additional <div> to render
Nice suggestion, I'll do the following experiments around what alex suggested
Agreeing with Alex, I hope Fred's experiments show "how" this one column can improve so massively.

Are the improvement numbers for a combined size/transfer column?
Flags: needinfo?(hkirschner)
stop calling getFormatedSize leads to worse perf
https://reviewboard.mozilla.org/r/204472/diff/1

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


But make shouldComponentUpdate returns false contribute most of the perf gain

* simple.netmonitor.requestsFinished -7.49% (med)
* complicated.netmonitor.requestsFinished -4.61% (med)

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


And I found cache l10n strings also bring 5% perf gain 

complicated.netmonitor.requestsFinished -5.81% (low)
complicated.netmonitor.requestsFinished -4.80% (med)

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

Will file another bug for this simple l10n win.
Depends on: 1423102
As DAMP results for l10n caching made little sense to me, I ran another set of try push to double check.
With custom base run, to prevent any noise coming from recent improvement/regression. And I get very different results,
except for the contentSize column removal.

# remove contentSize column
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=7fa029746c1363a424e4dc829244204264abc4c4&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1
  -7.8% complicated.requestsFinished

# shouldComponentUpdate false
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=a42826c638d363bb3bda1c8cebdf2e5f1f7fedf1&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1
  no significant change

# Remove getFormattedSize
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=2d8cad8994e8fba87b429799260aee597cb7bc90&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1
  no significant change

My conclusion so far is that it isn't clear what is the actual additional cost of a column.
I still have one option in mind. The additional DOM cost.

Here is another patch, where render() returns an empty <div>. I just pushed it so I don't have the results yet:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=fa2db108c733245b3c643d06aa26d11e79944e5a&framework=1
I so the render PR in comment 9 just returns null instead of return an empty <div>
https://hg.mozilla.org/try/rev/8f60835f85a846284ecf09302976e67a8cfa7400
the test might not meaningful since the module is not shown to the user, and react might do the trick to not re-render again.


For quick perf gain we can still disable one of columns (I prefer keep the transfered size since it reflect the actual network status) if it make sense.
I looked deeper into this with a couple of additional experiments.

# Remove the title attribute
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=86287acb37025a1bda9817485280e2cbbf8bdbab&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1
  No particular win

# Remove <div> children (the text, i.e. content size text element)
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=e431200f61d636d449d24a6cd23d36f1ee0345ad&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1
  No particular win

# Remove <div> className
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=a70abc0c8bfd173bd0b7b173b9a5f7562132a56f&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1
  6.3% win on complicated.netmonitor.requestsFinished

New conclusion is that it doesn't seem to be related to extra DOM elements/attributes, but more about how the columns are rendered via these classNames and CSS rules that match.

It may be interesting to see what in these rules make one column have so much impact on netmon performances:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/RequestList.css#78-92
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/RequestList.css#437-439
(In reply to Fred Lin [:gasolin] from comment #13)
> removed all column's width style brings -7.87%(high) for
> complicated.netmonitor.requestsFinished
> 
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=mozilla-
> central&newProject=try&newRevision=7a7a08689f0f6c1f952776adaebc191fdf55d61b&o
> riginalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec6
> 6500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTime
> Range=172800

Sounds promising. Can you attach the patch?

---

As far as removing columns is concerned (from product perspective, not performance perspective) we could remove the Transferred column if the following is true:

1) The size column renders a label 'cached' next to the size number if the request is loaded from the cache
2) The size column displays a tooltip with transferred size

We should file a followup (depending how this bug is resolved eventually)

Honza
Setting a width in pixels for content size, and only this column brings a 7% win on complicated.requestsFinished:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=ee78e217207bf50b8cf31d85d652fc406221488c&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800

I used a random width here, but is this viable? Can we set columns width in pixels? If yes, how much pixels/em should we use?
> > removed all column's width style brings -7.87%(high) for
> > complicated.netmonitor.requestsFinished
> 
> Sounds promising. Can you attach the patch?
> 
> ---

It's just a quick experiment comparing to only remove one of the content size or the transferred size column.
To proper deal the width we need to look it carefully and apply some react table layout to adjust the width wisely.
Product: Firefox → DevTools
Inactive, moving to the backlog (P3)
Honza
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.