1345939 - javascript: performance.now malfunctions / returns bad values, on AMD Phenom II

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[somnia]

Attachment: jsbug.htm

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161215004017

Steps to reproduce:

Run a short javascript program. File attached.
Wait for about 2 minutes. The text "pos" turns into "error:NEG".

Note: Wrong result ONLY on Windows 7 + Machine A (Phenom II 955, AMD 770). 
On two other machines (Intel) + Win7, results are OK. 
On Linux + Machine A, OK.
On Google Chrome + Win7 + Machine A, results OK.

On Firefox 52 + Win7 + Machine A, FAIL.
On Firefox Beta 53.0b1 64-bit + Win7 + Machine A, FAIL.
On Firefox DE 52.0a2 + Win7 + Machine A, FAIL.

Note: I have noticed this bug earlier, it is there for at least a year, if not much longer.


Actual results:

Wrong result of execution.

Wait for about 2 minutes. The text "pos" turns into "error:NEG" (because performance.now() returned a negative number). Afterwards, the number "tm" is 16 most of the time, indicating that performance.now() downgraded to video refresh instead of High Performance Timer.


Expected results:

The text "pos" should have remained visible. "error:NEG" should not be displayed. 
The number "tm" should be oscillating, usually around 4 (ms) on desktops. There is no reason for the number "tm" to be 16 (ms) if High Perfomance Timer is available.

Comment 1

[somnia]

Note: to reproduce the bug, restart Firefox before running the attached file. Otherwise, "tm" might already be 16 on start and the bug won't show itself.

Comment 2

[somnia]

Correction: performance.now() doesn't return a negative number. What the program is detecting is when performance.now() goes backward in time.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Honza, do you know what might be going on with the timers on this processor?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #3)
> Honza, do you know what might be going on with the timers on this processor?

What is the underlying implementation of performance.now()?

Comment 5

[Honza Bambas (:mayhemer)]

Note that TimeStamp::Now() is not protected on windows to go backward.  Should be added at [1] when TSC is not stable.

[1] https://dxr.mozilla.org/mozilla-central/rev/34585620e529614c79ecc007705646de748e592d/mozglue/misc/TimeStamp_windows.cpp#199

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

TimeStamp::Now() and compared to a timestamp taken at start of navigation:

https://dxr.mozilla.org/mozilla-central/source/dom/performance/Performance.cpp#146

The creation timestamp comes from here for a window:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDOMNavigationTiming.cpp#77

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #6)
> TimeStamp::Now() and compared to a timestamp taken at start of navigation:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/performance/Performance.
> cpp#146
> 
> The creation timestamp comes from here for a window:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/base/
> nsDOMNavigationTiming.cpp#77

Thanks.  I'll fix it in the windows impl for timestamp (comment 5).

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1

GetTickCount(64) is protected against going back when used on the same thread.  QueryPerfomanceCounter (on non-stable TSC) can jitter (and go back) and for some small amounts we ignore that jitter.  

This patch adds a simple |return lock:last = max(last, now())|-like atomic protection.

Note that sTimeStampLock is a fast spin lock, so it's presumed to be cheap.  Also, there is not much hardware with non-stable TSC these days.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8873091 [details] [diff] [review]
v1

Review of attachment 8873091 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is the correct fix, but I am happy to discuss.  Perhaps it'd be worthwhile to do anyway in combination with the new check suggested below?

::: mozglue/misc/TimeStamp_windows.cpp
@@ +199,5 @@
>    LARGE_INTEGER pc;
>    ::QueryPerformanceCounter(&pc);
> +
> +  if (!sHasStableTSC) {
> +    // This is a simple go-backward protection for faulty hardware

It's not clear to me that we're dealing with faulty hardware here.  The HasStableTSC check returns false for non-Intel CPUs, and comment 0 says the problem only happens on an AMD CPU.  Perhaps the check in HasStableTSC should be made more robust instead?  (And we might realize speedups on non-Intel CPUs too?)

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> It's not clear to me that we're dealing with faulty hardware here.  The
> HasStableTSC check returns false for non-Intel CPUs, and comment 0 says the
> problem only happens on an AMD CPU.  Perhaps the check in HasStableTSC
> should be made more robust instead?  (And we might realize speedups on
> non-Intel CPUs too?)

I must have misread that function.  I though that we return true for all Intels and check APM for non-Intels.  But it's opposite.

According [1] AMDs advert APM too, so no reason to claim "all AMDs don't have stable TSC, ever" IMO.

Still I think the don't-go-back logic is something we should enforce on non-APM systems.

[1] https://en.wikipedia.org/wiki/Time_Stamp_Counter#Implementation_in_various_processors

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> (In reply to Nathan Froyd [:froydnj] from comment #9)
> > It's not clear to me that we're dealing with faulty hardware here.  The
> > HasStableTSC check returns false for non-Intel CPUs, and comment 0 says the
> > problem only happens on an AMD CPU.  Perhaps the check in HasStableTSC
> > should be made more robust instead?  (And we might realize speedups on
> > non-Intel CPUs too?)
> 
> I must have misread that function.  I though that we return true for all
> Intels and check APM for non-Intels.  But it's opposite.

...where "opposite" here means "we return false for all non-Intel chips" and check APM for Intel, correct?  Just verifying we're on the same page here.

> According [1] AMDs advert APM too, so no reason to claim "all AMDs don't
> have stable TSC, ever" IMO.

I am confused by this statement.  I agree that we shouldn't just blindly assume that all AMD processors have a stable TSC, merely that we should check APM support *and* stable TSC support on at least AMD processors in addition to doing so on Intel chips.  So the logic would become:

  // Get cpuid CPU string
  ...
  if (!intel && !amd) {
    // assume stable TSC is not supported
    return false;
  }

  // Check APM/stable TSC bit
  ...

For AMD chips that support stable TSC values, this would also increase performance, which seems valuable.

> Still I think the don't-go-back logic is something we should enforce on
> non-APM systems.

I think that's probably valuable.  But we can do both in this bug, yes?

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> > (In reply to Nathan Froyd [:froydnj] from comment #9)
> > > It's not clear to me that we're dealing with faulty hardware here.  The
> > > HasStableTSC check returns false for non-Intel CPUs, and comment 0 says the
> > > problem only happens on an AMD CPU.  Perhaps the check in HasStableTSC
> > > should be made more robust instead?  (And we might realize speedups on
> > > non-Intel CPUs too?)
> > 
> > I must have misread that function.  I though that we return true for all
> > Intels and check APM for non-Intels.  But it's opposite.
> 
> ...where "opposite" here means "we return false for all non-Intel chips" and
> check APM for Intel, correct?  Just verifying we're on the same page here.

Yes.

> 
> > According [1] AMDs advert APM too, so no reason to claim "all AMDs don't
> > have stable TSC, ever" IMO.
> 
> I am confused by this statement.  I agree that we shouldn't just blindly
> assume that all AMD processors have a stable TSC, merely that we should
> check APM support *and* stable TSC support on at least AMD processors in
> addition to doing so on Intel chips.  So the logic would become:

yes!

> > Still I think the don't-go-back logic is something we should enforce on
> > non-APM systems.
> 
> I think that's probably valuable.  But we can do both in this bug, yes?

yes, we can.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v2

- hasStableTSC returns:
  - false for non-intel and non-AMD processors
  - false when there is no way to detect Advanced Power Management
  - false when APM claims the TSC is not constant-rate
  - true otherwise
- when we run on a hardware w/o a stable TSC QueryPerformanceCounter is protected against going backwards (I don't think we need to do this when TSC is advertised stable)


https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea8b9d8c485615280318e1e4b8436951323e807e

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8877125 [details] [diff] [review]
v2

Review of attachment 8877125 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thank you!

Comment 15

[Honza Bambas (:mayhemer)]

somnia, could you please verify if this this fixed the problem?  There are builds at https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ea8b9d8c485615280318e1e4b8436951323e807e/

thanks.

Comment 16

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: TimeStamps are pretty fundamental to a lot of things we do inside Gecko, and making them more robust seems reasonable.

Comment 17

[Gerry Chang [:gchang]]

Since 54 was released and this issue has been there for a while. We may not take this in 54 if we end up having 54 dot release.

Comment 18

[Honza Bambas (:mayhemer)]

Going to land w/o original problem verification.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db2da03c5fb
Protect TimeStamp::Now() from going backwards on hardware with non-stable TSC. r=nfroyd

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4db2da03c5fb

Comment 21

[Julien Cristau [:jcristau] (back April 22)]

Time going backwards sounds bad.  Honza, do you want this to bake in nightly for a bit or should we get it in beta asap?

Comment 22

[Honza Bambas (:mayhemer)]

I'd rather let this bake on m-c.  It's a sensitive change.  When something goes wrong, it's hard to identify that timestamp is the cause.  Also, we lived with this for number of releases ;)

Comment 23

[Julien Cristau [:jcristau] (back April 22)]

Alright, wontfix for 55, thanks!