Closed Bug 1421704 Opened 4 years ago Closed 4 years ago

Optimize / Delay History::NotifyVisited


(Firefox :: Migration, enhancement, P3)




Firefox 59
Tracking Status
firefox59 --- fixed


(Reporter: dthayer, Assigned: dthayer)


(Blocks 1 open bug)



(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
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: Pre-optimize 1 Pre-optimize 2 Post-optimize 1 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
Attachment #8934307 - Flags: review?(mak77) → review+
Comment on attachment 8934307 [details]
Bug 1421704 - Optimize NotifyVisited IPC to take batch

::: 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
Optimize NotifyVisited IPC to take batch r=mak
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.