Open Bug 1374382 Opened 7 years ago Updated 2 months ago

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

Categories

(Core :: Performance Engineering, enhancement, P3)

enhancement

Tracking

()

Performance Impact none
Tracking Status
firefox57 --- fix-optional

People

(Reporter: smaug, Unassigned)

References

Details

(Keywords: perf, perf:pageload)

Attachments

(1 file)

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)?
Summary: Consider to process DOMLinkAdded event using animation frame callback to setTimeout(, 0) → Consider to process DOMLinkAdded event using animation frame callback + setTimeout(, 0)
Whiteboard: [qf]
Component: Activity Streams: General → Bookmarks & History
Slowing down first paint by a couple of ms seems important enough to be qf:p2.
Whiteboard: [qf] → [qf:p2]
Keywords: perf
Whiteboard: [qf:p2] → [qf:i60][qf:p2]
Whiteboard: [qf:i60][qf:p2] → [qf:i60][qf:p1]
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
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.
Assignee: nobody → mconley
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……… ? ;)
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.
See Also: → 1352459, 1393924
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]
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.
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.
Assignee: mconley → nobody
Whiteboard: [qf:f60][qf:p1] → [qf:f60][qf:p3]
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]
Whiteboard: [qf:f60][qf:p3] → [qf:f61][qf:p3]
Component: Bookmarks & History → General
Whiteboard: [qf:f61][qf:p3] → [qf:f61][qf:p3][fxperf]
Flags: needinfo?(mconley)
Whiteboard: [qf:f61][qf:p3][fxperf] → [qf:f61][qf:p3][fxperf:p3]
Flags: needinfo?(mconley)
Whiteboard: [qf:f61][qf:p3][fxperf:p3] → [qf:p3][fxperf:p3]
Performance Impact: --- → P3
Whiteboard: [qf:p3][fxperf:p3] → [fxperf:p3]
Severity: normal → S3

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

Performance Impact: low → ?
Keywords: perf:pageload
Whiteboard: [fxperf:p3]

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.

Performance Impact: ? → none
Component: General → Performance Engineering
Product: Firefox → Core
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: