Open
Bug 1421926
Opened 6 years ago
Updated 2 years ago
Show one of content size or transferred size by default to reduce the requestsFinished time
Categories
(DevTools :: Netmonitor, defect, P3)
DevTools
Netmonitor
Tracking
(Not tracked)
NEW
People
(Reporter: gasolin, Unassigned)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
removed method,domain,contentSize from default columns did increase the performance complicated.netmonitor.requestsFinished -11.74% (high, 7486 > 6607) simple.netmonitor.requestsFinished - 3.48 (low) https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=2835957dbbfbb73d7750b83030949445ca2dbb88&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800 PR https://reviewboard.mozilla.org/r/194564/diff/20 Only remove contentSize or Only remove transfer Size column can improve complicated.netmonitor.requestsFinished time by 8% remove contentsize -8.24% (high, 7538 > 6917) https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=5b0de1b3edc6ca739098f0d058abf80106295064&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800 remove transfer Size -8.42% https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=39d1d5ca1483ba375260a83e80fdf002845a84fa&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
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
Reporter | ||
Comment 3•6 years ago
|
||
Nice suggestion, I'll do the following experiments around what alex suggested
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•6 years ago
|
||
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.
Comment 9•6 years ago
|
||
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
Reporter | ||
Comment 10•6 years ago
|
||
Thanks alex for further experiment. according to the above comment, returns an empty <div> bring -7.44%(high) perf gain https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=2b667a68c6ef4a781b5f04675c96fd0a7e705074&newProject=try&newRevision=fa2db108c733245b3c643d06aa26d11e79944e5a&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1 We could revisit this when we updated to react 16.2, which can return component with no wrap element https://reactjs.org/blog/2017/11/28/react-v16.2.0-fragment-support.html
Reporter | ||
Comment 11•6 years ago
|
||
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.
Comment 12•6 years ago
|
||
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
Reporter | ||
Comment 13•6 years ago
|
||
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&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800
Comment 14•6 years ago
|
||
(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
Comment 15•6 years ago
|
||
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?
Updated•6 years ago
|
Priority: -- → P2
Reporter | ||
Comment 16•6 years ago
|
||
> > 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.
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•