1421704 - Optimize / Delay History::NotifyVisited

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

[Alex Thayer [:alexical] (she/her)]

From Bug 1418443:

> Last thing, looks like we spend a lot of time in NotifyVisited. It has an
> AutoScriptBlocker and in general it looks expensive. That sounds like a very
> good candidate for optimization. it's also all an internal detail so we could
> change it more easily and, if I remember correctly, it's not critical for it
> to happen immediately, we could delay it by a little bit. That's what marks
> links as visited by changing their color. We could potentially dispatch the
> same runnable again, and in the second Run() call do the NotifyVisited, to
> give main-thread some breathing. We could also change SendNotifyVisited to a
> more batched message, passing an URIParams[] array...

Comment 1

[Alex Thayer [:alexical] (she/her)]

Surprisingly, just changing SendNotifyVisited to take a URIParams[] array seems to have an even bigger effect than I would expect. As expected, it reduces the time spent in NotifyVisited from around 15ms to around 5, but it also reduces the time spend outside of NotifyVisited from around 90ms to around 45ms. I'm linking a few profiles for reference:

https://perfht.ml/2AN6PQi Pre-optimize 1
https://perfht.ml/2AOfDFJ Pre-optimize 2
https://perfht.ml/2AMRRKm Post-optimize 1
https://perfht.ml/2AOJSwn Post-optimize 2

I'll be cleaning up the patch for review shortly - maybe I just goofed something but it appears to be doing its job of changing link colors well enough.

Comment 2

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1421704 - Optimize NotifyVisited IPC to take batch

During history import, sending NotifyVisited messages from the
chrome process to the content processes in order to change link
colors can take a significant portion of the parent process's
main thread time. Batching it seems to have very significant
results on jank time during history imports.

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

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8934307 [details]
Bug 1421704 - Optimize NotifyVisited IPC to take batch

https://reviewboard.mozilla.org/r/205214/#review213304

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8934307 [details]
Bug 1421704 - Optimize NotifyVisited IPC to take batch

https://reviewboard.mozilla.org/r/205214/#review213350

::: toolkit/components/places/History.cpp:579
(Diff revision 1)
> +      URIParams uri;
> +      SerializeURI(mURI, uri);
> +      uris.AppendElement(uri);

I suggest doing:

```
URIParams* p = uris.AppendElement();
SerializeURI(mURI, *p);
```

to avoid constructing a temporary `uri`, copy-constructing it into the array, and then destroying it.  Or perhaps just doing `uris.AppendElement(Move(uri))` to at least avoid the copy-construction.

::: toolkit/components/places/History.cpp:685
(Diff revision 1)
> +    SerializeURI(uri, serialized);
> +    aNotifyVisitedURIs.AppendElement(serialized);

Same comment applies here.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8934307 [details]
Bug 1421704 - Optimize NotifyVisited IPC to take batch

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cbcc8b4fa57
Optimize NotifyVisited IPC to take batch r=mak

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8cbcc8b4fa57