605680 - Stop deleting _overLinkDelayTimer property in urlbarBinding to prevent performance suck

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Drew Willcoxon :adw]

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

Comment 1

[Drew Willcoxon :adw]

Attachment: 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).

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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

Comment 3

[Drew Willcoxon :adw]

That all makes sense, thanks Gavin.

http://hg.mozilla.org/mozilla-central/rev/d6c5a710d01c