1374382 - Consider to process DOMLinkAdded event using animation frame callback + setTimeout(, 0)

Status: NEW

(Core :: Performance Engineering, enhancement, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

Right now http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/browser/modules/ContentLinkHandler.jsm#38 process DOMLinkAdded synchronously and that slows down first paint of a web page couple of milliseconds.
Could we do the actual work asynchronously using animationFrameCallback + setTimeout(, 0)?

Comment 1

[Andrew Overholt [:overholt]]

Slowing down first paint by a couple of ms seems important enough to be qf:p2.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I'm going to take a crack at this. I'm going to see if I can debounce all reactions to DOMLinkAdded and DOMLinkChanged, and use idleCallbacks.

Comment 3

[Ed Lee :Mardak]

Not sure if we want to tackle ContentMetaHandler at the same time:
https://searchfox.org/mozilla-central/source/browser/modules/ContentMetaHandler.jsm

In particular for ContentLinkHandler, I believe nanj ran into issues with tests when introducing FAVICON_PARSING_TIMEOUT:
https://searchfox.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm#261-264

But I suppose those are theoretically fixed in landing bug 1352459, so adding debouncing before we even fully process DOM{Link,Meta}{Added,Changed} events shouldn't be too bad……… ? ;)

Comment 4

[Ed Lee :Mardak]

Oh, one other thing to note about ContentLinkHandler is that the TIMEOUT there is 100ms whereas ContentMetaHandler has 1000ms. I believe the smaller number for Link is because delaying it longer has visual regressions / more noticeable popping in of the favicon in the tab.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1374382 - [WIP] Defer DOMLinkAdded and DOMLinkChanged reactions until an idlecallback.

Review commit: https://reviewboard.mozilla.org/r/216958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216958/

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8947190 [details]
Bug 1374382 - [WIP] Defer DOMLinkAdded and DOMLinkChanged reactions until an idlecallback.

https://reviewboard.mozilla.org/r/216958/#review222816


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/general/browser_favicon_change.js:9
(Diff revision 1)
>  "use strict";
>  
>  const TEST_URL = "http://mochi.test:8888/browser/browser/base/content/test/general/file_favicon_change.html";
>  
>  add_task(async function() {
> +  debugger;

Error: Unexpected 'debugger' statement. [eslint: no-debugger]

Comment 7

[Nan Jiang [:nanj]]

Both mak and I ran into a few intermittent test failures when we first introduced the debouncing logic to ContentLinkHandler, as the original synchronous behavior expects the instant/low-latency favicon loads. Certain tests might need some further tweaks for this change.

Yes, ContentMetaHandler could be a perfect use case for this optimization because, unlike favicon parsing, it has no visual effect on the page, and could be done in the idle callback.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8947190 [details]
Bug 1374382 - [WIP] Defer DOMLinkAdded and DOMLinkChanged reactions until an idlecallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216958/diff/1-2/

Comment 9

[Mike Conley (:mconley) (:⚙️)]

My latest WIP patch here defers the processing of DOMLinkAdded and DOMLinkChanged events until after the page fires a load event (and even after then, only when there's idle time).

Here's the talos test comparison:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1704d48ed49d46c5e01a4a7b84c2abd1a108a593&newProject=try&newRevision=1376cd8eab9a&framework=1

I'm not seeing any wins here. If we were spending an inordinate amount of time processing these events, I would have expected wins in tp5 and tp6.

So I'm going to shelve this work for now, and take the liberty of downgrading the priority of this bug from qf:p1 to qf:p3, since we should be investing time going after bigger, more measurable improvements.

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8947190 [details]
Bug 1374382 - [WIP] Defer DOMLinkAdded and DOMLinkChanged reactions until an idlecallback.

https://reviewboard.mozilla.org/r/216958/#review223310


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/content.js:676
(Diff revision 2)
> -ContentLinkHandler.init(this);
> +var contentLinkHandler;
> +
> +if (content === content.top) {
> + contentLinkHandler = new ContentLinkHandler(this);
> +}
> +//ContentLinkHandler.init(this);

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Now that we have more robust page load measurements with browsertime, we might want to try this again.

Comment 12

[Frank Doty [:fdoty]]

This seems to be more of a task rather than defect. For tracking purposes, we're placing this under Performance Engineering as a task, but Performance Impact is none.

Additional note, the patch mentioned in comment 10 was in review board and is no longer accessible.