625030 - Enable high resolution counter in System::getTimer

Status: RESOLVED FIXED

(Tamarin Graveyard :: API, enhancement, P3)

Description

[Rob Winchell]

On Windows, Date::getTime() uses the API GetSystemTime(), which only has a 10-15ms resolution. On OSX/Linux, gettimeofday() is used which results in a 1ms resolution, so it's possible to see very different timing results on similar hardware between Windows and OSX/Linux systems.

It would be be useful to have a PerformanceCounter class that would return a counter value in nanoseconds or microseconds. On Windows this would be implemented using QueryPerformanceFrequency/QueryPerformanceCounter, and on OSX/Linux using clock_getres/clock_gettime.

Comment 1

[Lars T Hansen]

If you provide nanoseconds a Number can represent values exactly for up to 104 days after startup (2^53 bits).  Since this is an internal API we can probably call that good enough.  I suspect ns would be desirable because we're now to the point where thousands of instructions can be executed in a microsecond.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(Just to get it on the record:) Unless [until?] doubles have no overhead compared to ints, I do not see a good reason to tie ourselves to the overhead of heap-allocating doubles when accessing the counter.  The difference between the two would probably not matter for my current use case, but this is meant for internal-use, so I would favor minimizing the overhead introduced by accessing the counter over maximizing programmer-efficiency.

We could of course support both interfaces, no?  The internal API could provide one getter that returns a uint counter (probably measured in microseconds) and another getter that provides a Number in nanoseconds.

Comment 3

[Lars T Hansen]

Doubles require no heap allocation if used in properly typed code, so impose no overhead from heap allocation.  The way to ensure this would be to type the accessor as "Number" and ensure that code that obtains performance counts stores those counts in typed locations.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

I retract my objection; heap-allocation was the main overhead I was
concerned about.  (That is, I'm not concerned with the overhead of
floating-point arithmetic.)

Comment 5

[Rob Winchell]

High resolution timers are already implemented in System::getTimer, but are not enabled. They are currently ifdef'ed out by PERFORMANCE_GETTIMER. Felix reports that enabling this gives much better results on Windows.

1. Is there any reason not to enable this?
2. Should this functionality be exposed through a PerformanceCounter class, or is the current system fine?

Comment 6

[Lars T Hansen]

(In reply to comment #5)
> High resolution timers are already implemented in System::getTimer, but
> are not enabled. They are currently ifdef'ed out by PERFORMANCE_GETTIMER.
> Felix reports that enabling this gives much better results on Windows.
> 
> 1. Is there any reason not to enable this?

I don't remember.

> 2. Should this functionality be exposed through a PerformanceCounter class, or
> is the current system fine?

If you mean the Date class then no, that *must* have millisecond resolution for compatibility reasons.  We have open bugs on it delivering fractions-of-millisecond resolution on Mac, users get really confused.

I don't see the reason for a PerformanceCounter class, really; how about simply a single getter, System.nanosecondTimer ?  And add System.nanosecondTimerResolution if necessary?

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #6)
> (In reply to comment #5)
> > High resolution timers are already implemented in System::getTimer, but
> > are not enabled. They are currently ifdef'ed out by PERFORMANCE_GETTIMER.
> > Felix reports that enabling this gives much better results on Windows.
> > 
> > 1. Is there any reason not to enable this?
> 
> I don't remember.
> 
> > 2. Should this functionality be exposed through a PerformanceCounter class, or
> > is the current system fine?
> 
> If you mean the Date class then no, that *must* have millisecond resolution for
> compatibility reasons.  We have open bugs on it delivering
> fractions-of-millisecond resolution on Mac, users get really confused.

Rob might be talking about System.getTimer when he says "the current system"

Just to clarify, System.getTimer uses millisecond unit of measurement, it is higher resolution than the Date class, because on Windows the Date class is not consistently reporting values at a finer than 15ms grain.

Comment 8

[Rob Winchell]

Right, I just meant System.getTimer as the current system. No need to mess with Date.

Comment 9

[Dan Smith]

System::getTImer can implement high resolution timers.  Would it be useful to make this configurable via a MMS config setting rather than having to produce a custom build?   Is a higher resolution timer desirable for HalfMoon?

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

tlt notes:

"""
just stumbled upon this, i suspect some ppl here might be interested: The time stamp counter in newer processors may support an enhancement, referred to as invariant TSC. Processor’s support for invariant TSC is indicated by CPUID.80000007H:EDX[8].

The invariant TSC will run at a constant rate in all ACPI P-, C-. and T-states. This is the architectural behavior moving forward. On processors with invariant TSC support, the OS may use the TSC for wall clock timer services (instead of ACPI or HPET timers). TSC reads are much more efficient and do not incur the overhead associated with a ring transition or access to a platform resource.
"""

I believe this implies that on sufficiently advanced processor architectures, we should not have to worry about cpu affinity and what not (hooray).

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: trivial patch A: enable #define PERFORMANCE_GETTIMER

(posting this here for completeness for people following Bug 698721.)

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch N: getNanosecondTimer

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch N v2: getNanosecondTimer

Revised patch to only add getNanosecondTimer to the System class, not to the shell toplevel, due to performance issues documented in Bug 698721, comment 22.

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 571580 [details] [diff] [review]
patch N v2: getNanosecondTimer

I'm pretty sure we want this available at least in the avmshell, regardless of how Bug 698721 turns out.

Comment 15

[Lars T Hansen]

Comment on attachment 571580 [details] [diff] [review]
patch N v2: getNanosecondTimer

At least the first cast to double in getNanosecondTimer is redundant, I suspect they both are.

Comment 16

[Tamarin Bot]

changeset: 6707:db3ce7ce9b9d
user:      Felix S Klock II <[e-mail]>
summary:   Bug 625030: Add System.getNanosecondTimer method (r=lhansen).

http://hg.mozilla.org/tamarin-redux/rev/db3ce7ce9b9d

Comment 17

[Tamarin Bot]

changeset: 6708:04d83c8f62e3
user:      Felix S Klock II <[e-mail]>
summary:   Bug 625030: generated code for changeset 6707:db3ce7ce9b9d.

http://hg.mozilla.org/tamarin-redux/rev/04d83c8f62e3

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

I removed the redundant casts that Lars noted in comment 15.

The addition of System.getNanosecondTimer should be good enough to resolve this for now, because:

-- The platform dependencies (i.e. QueryPerformanceFrequency/QueryPerformanceCounter, clock_getres/clock_gettime mentioned in comment 0) are factored into the (pre-existing) VMPI methods that this is layered on top of.

-- So if System.getNanosecondTimer is found to be producing inadequate results, it should almost certainly be due to problems in VMPI_getPerformanceCounter and/or VMPI_getPerformanceFrequency, and therefore separate bugs can be filed for those.

There is the orthogonal question of whether we should add the ability to the JIT to emit calls to read a processor's timestamp counter (TSC) on architectures where such is available, as alluded to in comment 10.  But that should be considered Future work that we will get to if we find it necessary.  (IMO we're not at the point yet where we need such fine-grained results.)