Bug 605680 made a large improvement to urlbarBindings's setOverLink performance, but there's room for more. Once that bug was fixed, I did some more Shark profiling with the test in bug 605680 comment 0 and saw that repeated calls to set/clearTimeout and related calls like nsScriptSecurityManager::CanAccess were taking up nearly half the test time. It would be better if instead of clearing and setting a timeout for each call to setOverLink, rapid calls subsequent to the first were no-ops. This patch replaces set/clearTimeout with set/clearInterval. The first time setOverLink is called, it sets the interval. If setOverLink isn't called again before the interval fires, then it's just like before: the over-link is updated, and the interval is cleared. This is what happens when the user mouses over a single link. But if setOverLink is called again before the interval fires, the interval sees that and doesn't clear itself, and as long as the interval is set, setOverLink is basically a no-op. When the interval sees that setOverLink has not been called since the last time it was fired, it updates the over-link and clears itself. This is what happens when the user mouses over many links in rapid succession. The downside is that when setOverLink is called in rapid succession, the delay for showing or hiding the over link when the user finally stops on a link is non-constant: it's in the range [100ms, 200ms). And I think this approach is not as easy to understand as the set/clearTimeout one. The upside is that my test on my debug build goes from 215ms to 21ms (90% win). In my opt build, 54ms to 2ms (96% win). 2ms is practically nothing and is actually faster than my test on 3.6, where it takes 45ms.

(In reply to comment #0) > The upside is that my test on my debug build goes from 215ms to 21ms (90% win). > In my opt build, 54ms to 2ms (96% win). 2ms is practically nothing and is > actually faster than my test on 3.6, where it takes 45ms. That's a measure of the time actually spent under setOverLink, right? Having the interval firing constantly during the mousing-over-in-rapid-succession case will have an impact as well... probably not as much as constantly setting/clearing the timeout, but I don't think it's a completely fair comparison.

The test simply grabs Date.now() before starting and after ending and compares the two. There's only one event loop, so that should capture the time in the interval too, no?

"before starting and after ending" what? Measuring the time spent under setOverLink doesn't measure the time spent running the code in the timeout (or interval) function, because setOverLink returns before that happens. If your measurements include time spent in the timeout/interval functions and also time spent managing the interval itself, then my point is moot. But if you're only measuring setOverLink, moving work from setOverLink to the interval function is going to overestimate the win. In other words, what we have now for the case of many rapid calls to setOverLink is: - setOverlink is called a lot, and does a lot of work setting/clearing timeouts - those timeouts get cleared before running, so they don't actually run The overhead there is obvious - time spent setting/clearing timeouts. So that's what your patch addresses. With it, we have: - setOverLink is called a lot, but doesn't do much work (sets the interval on the first call, and then essentially does nothing on subsequent ones) - the interval code is also running a lot (firing constantly), but doesn't really do any work until setOverLink stops being called. If there was a lot of overhead to the interval running constantly, you wouldn't know by just measuring time spent under setOverLink. In theory, it could outweigh the benefits of not having to set/clear the timeouts. That's unlikely, so your patch is probably still a net win, but we should be clear about what's being measured. The best way to get a big-picture view would be to run a profile that includes the DOM interval firing/management code (and the nsITimer code it depends on, etc.) with and without your patch.

I understand what you're saying. My point is that I'm not measuring time spent only under setOverLink. I'm measuring whatever goes through the event loop during the test. But, even though the intervals are scheduled during the test, I shouldn't assume that the event loop actually gets to all of them or even any of them during the test. I'll try to get better numbers.

Ah, I see, I misunderstood your comment. Very well then!

It's hard to tell how much time the intervals are taking using Shark. So I wrote a dtrace script that measures the amount of time from when nsTimerEvent::Run is entered to when it returns. It turns out my test, which calls setOverLink as fast as it can, triggers the interval only twice, both times after the test has finished, no matter whether it calls setOverLink 1,000 times or 100,000 times. On my opt build each interval duration is only ~0.2ms (not counting the time to update the over-link during the final interval, which both this approach and the current one with set/clearTimeout have to pay). I don't trust my dtrace skills, so I also added PR_Interval calculations in nsTimerEvent::Run and nsGlobalWindow::TimerCallback, and those verified the dtrace timings. If I modify the test to call setOverLink every 99ms, then the interval is called as many times as setOverLink is called, but each duration is still < 1ms. I guess that's not hard to believe, since the interval returns early every time except the final time. But apparently the overhead for firing intervals/timeouts/timers is low. Does that match your experience, Gavin?

Whoops, didn't mean to leave in those measurement-related changes.

Comment on attachment 491032 [details] [diff] [review] patch 2.1 (unbitrotted) sorry for the delay (http://hg.mozilla.org/mozilla-central/rev/8af7288622ea bitrotted this again)

Thanks Gavin! http://hg.mozilla.org/mozilla-central/rev/9ba5e353af1b