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

RESOLVED FIXED in mozilla16

Status

()

Core
IPC
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: adbugz, Assigned: adbugz)

Tracking

unspecified
mozilla16
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

5 years ago
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.
Attachment #615111 - Flags: review?(benjamin)

Comment 2

5 years ago
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.
Attachment #615111 - Flags: review?(jones.chris.g)
Attachment #615111 - Flags: review?(benjamin)
Attachment #615111 - Flags: review-
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.
Attachment #615111 - Flags: review?(jones.chris.g) → review-
(Assignee)

Comment 4

5 years ago
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.
Attachment #615111 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
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/
Attachment #617228 - Flags: review?(jones.chris.g)
Attachment #617228 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 629557 [details] [diff] [review]
For checkin

Same patch with title and author
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Assignee: nobody → adbugz
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e32a81f9dd

Sorry for the delay...
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/47e32a81f9dd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.