Closed Bug 1421704 Opened 2 years ago Closed 2 years ago
Optimize / Delay History::Notify
59 bytes, text/x-review-board-request
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.
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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/8cbcc8b4fa57 Optimize NotifyVisited IPC to take batch r=mak
You need to log in before you can comment on or make changes to this bug.