Closed Bug 1421704 Opened 2 years ago Closed 2 years ago

Optimize / Delay History::NotifyVisited

Categories

(Firefox :: Migration, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: dthayer, Assigned: dthayer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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...
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
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.
No longer blocks: 1421701
Comment on attachment 8934307 [details]
Bug 1421704 - Optimize NotifyVisited IPC to take batch

https://reviewboard.mozilla.org/r/205214/#review213304
Attachment #8934307 - Flags: review?(mak77) → review+
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.
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cbcc8b4fa57
Optimize NotifyVisited IPC to take batch r=mak
https://hg.mozilla.org/mozilla-central/rev/8cbcc8b4fa57
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.