Closed
Bug 1421704
Opened 7 years ago
Closed 7 years ago
Optimize / Delay History::NotifyVisited
Categories
(Firefox :: Migration, enhancement, P3)
Firefox
Migration
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 | ||
Updated•7 years ago
|
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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 4•7 years ago
|
||
mozreview-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.
Comment hidden (mozreview-request) |
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cbcc8b4fa57
Optimize NotifyVisited IPC to take batch r=mak
Comment 7•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in
before you can comment on or make changes to this bug.
Description
•