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

NEW
Unassigned

Status

()

Firefox
Bookmarks & History
P2
normal
11 months ago
3 months ago

People

(Reporter: smaug, Unassigned)

Tracking

({perf})

unspecified
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional)

Details

(Whiteboard: [qf:f61][qf:p3])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
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)?
(Reporter)

Updated

11 months ago
Summary: Consider to process DOMLinkAdded event using animation frame callback to setTimeout(, 0) → Consider to process DOMLinkAdded event using animation frame callback + setTimeout(, 0)
(Reporter)

Updated

11 months ago
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]
status-firefox57: --- → fix-optional
Priority: -- → P2

Updated

6 months ago
Whiteboard: [qf:p2] → [qf:i60][qf:p2]
Whiteboard: [qf:i60][qf:p2] → [qf:i60][qf:p1]

Updated

5 months ago
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

Comment 3

4 months ago
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

4 months ago
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: → bug 1352459, bug 1393924
Comment hidden (mozreview-request)

Comment 6

4 months ago
mozreview-review
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

4 months ago
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 hidden (mozreview-request)
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 10

4 months ago
mozreview-review
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]

Updated

3 months ago
Whiteboard: [qf:f60][qf:p3] → [qf:f61][qf:p3]
You need to log in before you can comment on or make changes to this bug.