655413 - Firefox locks system timer resolution to 1000Hz once Flash is used once

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[adbugz]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110506 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110506 Firefox/6.0a1

To see what is this about, please check these similar Chromium issues:

http://code.google.com/p/chromium/issues/detail?id=2039
http://code.google.com/p/chromium/issues/detail?id=46531
http://code.google.com/p/chromium/issues/detail?id=81693

(If you're going to do comparisons, notice the latest one isn't fixed yet)

In short, timeBeginPeriod(1) is being called by Firefox when Flash is used for the first time by any page, and never released.

The culprit is some code in xul.dll. From a quick look of mxr.mozilla.org and the corresponding disassembly, it's most likely this line: http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/time_win.cc#273

A binary patch replacing 0x538A5C240C568B74240C3A5E2C741784DB6A01 with 0x538A5C240C568B74240C3A5E2C741784DB6A10 workarounds the issue (changing the last byte, which is the parameter passed to timeBeginPeriod(), from 1 to 16)

To test this, you may use this free SysInternals utility: http://technet.microsoft.com/en-us/sysinternals/bb897568

Now this is tricky because Flash can also increase the resolution on its own. However, current versions (tested with 10.2.153.1) are good at releasing it when it's not needed (in particular, when no Flash objects are visible).

Apparently other plugins (such as Silverlight) don't have this problem: once they stop being used, the system timer resolution is lowered again.

Reproducible: Always

Comment 1

[adbugz]

Attachment: Just disable the offending function

This sure beats a binary patch. I took a quick look at the current Chromium source, this section is nowhere to be found anymore.

Comment 2

[Benjamin Smedberg]

Comment on attachment 615111 [details] [diff] [review]
Just disable the offending function

cjones, do you know if this has any effects on timestamp or anything else which we might care about? I tend to think that if we're going to remove this functionality, we should not comment it out but remove it completely (and the callers). So I'm going to mark r- just for the code bits, but I'd to know if there are side effects we might be missing.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 615111 [details] [diff] [review]
Just disable the offending function

This code still exists in upstream chromium, it's just been refactored.

Our Windows TimeStamp code prefers to use QPC but falls back to GetTickCount() in some situations.  I'm not sure whether the values reported by GTC are affected by the resolution of timeBeginPeriod().

At any rate, Gecko timing code shouldn't rely on side effects from the imported chromium code.  I'm 100% fine with disabling this code, but agree with bsmedberg, not like this.

Comment 4

[adbugz]

Attachment: More aggressive code removal

Tested compile (default build) and fix of the symptoms. Shaves 1.5 KB off xul.dll.

Comment 5

[adbugz]

Comment on attachment 617228 [details] [diff] [review]
More aggressive code removal

Ping?

Today I did measure this on a laptop, it was about an 8% battery life gain. I don't know if that's typical across different machines, but there's a reason CPU manufacturers are pushing for these kind of fixes.¹

¹ http://software.intel.com/en-us/articles/cpu-power-utilization-on-intel-architectures/

Comment 6

[adbugz]

Attachment: For checkin

Same patch with title and author

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47e32a81f9dd

Sorry for the delay...

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47e32a81f9dd