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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b7

People

(Reporter: adw, Assigned: adw)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) (deleted) — 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)
Attached patch patch 2 (deleted) — Splinter Review
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)
Attachment #484577 - Flags: review?(gavin.sharp)
Attachment #484577 - Flags: review+
Attachment #484577 - Flags: approval2.0+
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).
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
Summary: Remove _overLinkDelayTimer field from urlbar binding → Stop deleting _overLinkDelayTimer property in urlbarBinding to prevent performance suck
Keywords: perf
Blocks: 600635
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: