If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Stop deleting _overLinkDelayTimer property in urlbarBinding to prevent performance suck

RESOLVED FIXED in Firefox 4.0b7

Status

()

Firefox
Address Bar
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

({perf})

Trunk
Firefox 4.0b7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 484565 [details] [diff] [review]
patch

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

7 years ago
Created attachment 484577 [details] [diff] [review]
patch 2

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).
(Assignee)

Comment 3

7 years ago
That all makes sense, thanks Gavin.

http://hg.mozilla.org/mozilla-central/rev/d6c5a710d01c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
(Assignee)

Updated

7 years ago
Summary: Remove _overLinkDelayTimer field from urlbar binding → Stop deleting _overLinkDelayTimer property in urlbarBinding to prevent performance suck
Keywords: perf
(Assignee)

Updated

7 years ago
Blocks: 600635

Updated

7 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.