Closed
Bug 605680
Opened 14 years ago
Closed 14 years ago
Stop deleting _overLinkDelayTimer property in urlbarBinding to prevent performance suck
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 4.0b7
People
(Reporter: adw, Assigned: adw)
References
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Gavin
:
review+
Gavin
:
approval2.0+
|
Details | Diff | Splinter Review |
I've been doing some performance testing on urlbarBinding's setOverLink. I have a test that calls setOverLink("foo") and setOverLink("") back-to-back many times very quickly. It simulates quickly mousing over a long list of links in a web page, bookmarks in a bookmarks menu, and tabs in the all-tabs menu, and leaving the mouse pointer on a page while scrolling it. (Same test as bug 597338 comment 7.)
If I comment out the entire body of setOverLink, running the test 1000 times on my debug build takes 5ms. Without modification on trunk, ~650ms. All this test should trigger is setting and clearing the _overLinkDelayTimer over and over. So if I simplify setOverLink to do only this:
if (this._overLinkDelayTimer) {
clearTimeout(this._overLinkDelayTimer);
delete this._overLinkDelayTimer;
}
this._overLinkDelayTimer = setTimeout(function () {}, 100);
It takes ~580ms.
At first I thought it's the timeout implementation that's slow, but when I profiled with Shark I saw nsXBLProtoImplField::InstallField taking up 60% of the test time. When I remove the _overLinkDelayTimer field and re-run the test, it takes ~320ms, a 51% improvement over trunk.
So apparently we should avoid non-read-only XBL fields, especially when they can be set in tight loops like this one can...
Attachment #484565 -
Flags: review?(dao)
Assignee | ||
Comment 1•14 years ago
|
||
Gavin says it's because the property is deleted, so the binding falls back to the XBL proto again... and again and again. Instead of removing the field, setting the prop to null works, and for some reason it's actually a bigger win: ~215ms (67% improvement over trunk).
Attachment #484565 -
Attachment is obsolete: true
Attachment #484577 -
Flags: review?(gavin.sharp)
Attachment #484565 -
Flags: review?(dao)
Updated•14 years ago
|
Attachment #484577 -
Flags: review?(gavin.sharp)
Attachment #484577 -
Flags: review+
Attachment #484577 -
Flags: approval2.0+
Comment 2•14 years ago
|
||
It's likely a bigger win because you avoid the resolve hook entirely (rather than hitting it and having it bail out before calling InstallField because protoBinding->FindField returns null).
Assignee | ||
Comment 3•14 years ago
|
||
That all makes sense, thanks Gavin.
http://hg.mozilla.org/mozilla-central/rev/d6c5a710d01c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Assignee | ||
Updated•14 years ago
|
Summary: Remove _overLinkDelayTimer field from urlbar binding → Stop deleting _overLinkDelayTimer property in urlbarBinding to prevent performance suck
Updated•14 years ago
|
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7
You need to log in
before you can comment on or make changes to this bug.
Description
•