Last Comment Bug 655413 - Firefox locks system timer resolution to 1000Hz once Flash is used once
: Firefox locks system timer resolution to 1000Hz once Flash is used once
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: adbugz
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-06 16:26 PDT by adbugz
Modified: 2012-07-23 14:53 PDT (History)
9 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Just disable the offending function (546 bytes, patch)
2012-04-14 21:46 PDT, adbugz
cjones.bugs: review-
benjamin: review-
Details | Diff | Splinter Review
More aggressive code removal (2.60 KB, patch)
2012-04-21 08:53 PDT, adbugz
cjones.bugs: review+
Details | Diff | Splinter Review
For checkin (2.14 KB, patch)
2012-06-02 23:14 PDT, adbugz
no flags Details | Diff | Splinter Review

Description adbugz 2011-05-06 16:26:54 PDT
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 2012-04-14 21:46:54 PDT
Created attachment 615111 [details] [diff] [review]
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-16 05:56:10 PDT
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 2012-04-16 19:14:00 PDT
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 2012-04-21 08:53:07 PDT
Created attachment 617228 [details] [diff] [review]
More aggressive code removal

Tested compile (default build) and fix of the symptoms. Shaves 1.5 KB off xul.dll.
Comment 5 adbugz 2012-05-08 14:11:09 PDT
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 2012-06-02 23:14:22 PDT
Created attachment 629557 [details] [diff] [review]
For checkin

Same patch with title and author
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-06-09 14:16:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e32a81f9dd

Sorry for the delay...
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-06-09 19:45:18 PDT
https://hg.mozilla.org/mozilla-central/rev/47e32a81f9dd

Note You need to log in before you can comment on or make changes to this bug.