1. On systems where HasStableTSC() returns true, for an old TimeStamp ts_old obtained from Now(), (TimeStamp::Now() - ts_old) may differ from (TimeStamp::NowLoRes() - ts_old) by much more than the indicated lower precision of 15.6 ms. 2. On systems where HasStableTSC() returns false, for a recent TimeStamp ts_recent from Now(), a difference of durations is not likely to have high precision, due to fallback to the GetTickCount() timestamps. e.g (TimeStamp::Now() - ts_old) - (ts_recent - ts_old) This affects DOMHighResTimeStamp. 3. For two old TimeStamps from Now() ts_old1 and ts_old2, the duration ts_old2 - ts_old1 will change if/when sUseQPC is set to false. This is due to ts_old maintaining separate timestamps from GetTickCount() and QueryPerformanceCounter() time sources. I suspect an implementation that uses a single timestamp and periodically updates a difference for converting between the two sources, more similar to that prior to changes in bug 822490, would behave more as expected. May need to be careful about maintaining monotonicity of each of Now() and NowLoRes() (individually, not wrt each other), and finding another solution to the wake up issue described in bug 822490 comment 0.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #0) > 1. On systems where HasStableTSC() returns true, for an old TimeStamp ts_old > obtained from Now(), (TimeStamp::Now() - ts_old) may differ from > (TimeStamp::NowLoRes() - ts_old) by much more than the indicated lower > precision of 15.6 ms. Yes, that is known, GTC may jump in 15.6*N leaps or the 15.6ms is not 15.6 on that particular platform, there is not much to do about that. What is your exact concern? > > 2. On systems where HasStableTSC() returns false, for a recent TimeStamp > ts_recent from Now(), a difference of durations is not likely to have high > precision, due to fallback to the GetTickCount() timestamps. > e.g (TimeStamp::Now() - ts_old) - (ts_recent - ts_old) > This affects DOMHighResTimeStamp. I don't much follow.. What is the concern here? When there is no QPC, we don't have much better timer then GTC, which is stable (more or less - as above) and also very well performing (fast to call). But has lower resolution. What is the effect on DOMHighResTimeStamp? Please be more detailed. Or, would you suggest a different fallback/reference timer, e.g. getSystemTimeAsFileTime or timeGetTime (former preferred)? That is also relatively reliable and is also (on some platforms) sensitive to beginPeriod(1) that may be a double-blade tho... Honestly, for the previous implementation, getSystemTimeAsFileTime was used. > > 3. For two old TimeStamps from Now() ts_old1 and ts_old2, the duration > ts_old2 - ts_old1 will change if/when sUseQPC is set to false. We can have a very simple way to fix this, when QPC is found in both the previously taken TimeStamps, use it to calculate TimeDuration regardless the sUseQPC flag being false. I'll file a bug. > > This is due to ts_old maintaining separate timestamps from GetTickCount() and > QueryPerformanceCounter() time sources. Yes, and that is a prerequisite to have NowLoRes() that is pretty important on some windows platform with slow QPC. > (In reply to Karl Tomlinson (needinfo?:karlt) from comment #0) > I suspect an implementation that uses a single timestamp and periodically > updates a difference for converting between the two sources, more similar to > that prior to changes in bug 822490, would behave more as expected. May need > to be careful about maintaining monotonicity of each of Now() and NowLoRes() > (individually, not wrt each other), and finding another solution to the wake > up issue described in bug 822490 comment 0. I'm strongly against this. That code was badly performing (slow), wasting resources, using heavy locking, fixing one bug opened one or two others.. no. I really don't want to do that step back. I'd rather change the code to use QPC on stable TSC or getSystemTimeAsFileTime when not. That would simplify stuff much more, no calibration/fault detection at all.

(In reply to Honza Bambas (:mayhemer) from comment #1) > Honestly, for the previous implementation, getSystemTimeAsFileTime was used. Err... no, GTC was used. getSystemTimeAsFileTime is however used for PR_IntervalNow().

Bug 1034638.

(In reply to Honza Bambas (:mayhemer) from comment #1) > (In reply to Karl Tomlinson (needinfo?:karlt) from comment #0) > > 1. On systems where HasStableTSC() returns true, for an old TimeStamp ts_old > > obtained from Now(), (TimeStamp::Now() - ts_old) may differ from > > (TimeStamp::NowLoRes() - ts_old) by much more than the indicated lower > > precision of 15.6 ms. > > Yes, that is known, GTC may jump in 15.6*N leaps or the 15.6ms is not 15.6 > on that particular platform, there is not much to do about that. What is > your exact concern? My concern is that the difference is arbitrarily larger than the GTC jumps. (TimeStamp::Now() - ts_old) uses only QPC values. (TimeStamp::NowLoRes() - ts_old) uses only GTC values. AFAIK there is nothing to ensure that QPC and GTC values remain in sync. Clock skew between the sources can lead to arbitrarily large differences. This means that durations involving NowLoRes() TimeStamps should not be compared with those involving only Now() TimeStamps. I think these comparisons could be made reasonable, but if we choose not to do that, then the durations should have different types to prevent clients falling into this trap. > > 2. On systems where HasStableTSC() returns false, for a recent TimeStamp > > ts_recent from Now(), a difference of durations is not likely to have high > > precision, due to fallback to the GetTickCount() timestamps. > > e.g (TimeStamp::Now() - ts_old) - (ts_recent - ts_old) > > This affects DOMHighResTimeStamp. > > I don't much follow.. What is the concern here? When there is no QPC [...], we > don't have much better timer then GTC Yes, if we don't have QPC, then we need to use something else and GTC may be the appropriate something else. But my concern is for the case when we do have QPC. The "Check QPC is sane before using it." and |overflow| test in CheckQPC compares deltas from QPC and GTC time sources against a fixed/absolute threshold. Clock skew between the sources will mean that as deltas become large, we can expect the threshold to be exceeded. QPC will be used for small deltas and GTC for large deltas. > What is the effect on DOMHighResTimeStamp? Please be more detailed. DOMHighResTimeStamp uses a reference high res TimeStamp from Now() on page load. Subsequent timestamps are returned to content as durations from the page load TimeStamp. Initially these durations will be based on QPC, but, once accumulated clock skew reaches the threshold, they will be based on GTC. GTC durations will not have the desired precision, even though QPC is available. Also, when durations change from QPC to GTC, there is the possibility for a later TimeStamp to produce a shorter duration from page load. i.e. monotonicity is not provided. > Or, would you suggest a different fallback/reference timer, e.g. > getSystemTimeAsFileTime or timeGetTime (former preferred)? That is also > relatively reliable and is also (on some platforms) sensitive to > beginPeriod(1) that may be a double-blade tho... That wasn't my concern, and I don't know enough to comment on these options. > > 3. For two old TimeStamps from Now() ts_old1 and ts_old2, the duration > > ts_old2 - ts_old1 will change if/when sUseQPC is set to false. > > We can have a very simple way to fix this, when QPC is found in both the > previously taken TimeStamps, use it to calculate TimeDuration regardless the > sUseQPC flag being false. That particular case may be fixable, but the problem remains that for durations covering some overlapping time period, some are using QPC and some GTC. Ideally we'd effectively record the time when switching time sources, and use a consistent source on either side of that time. > > This is due to ts_old maintaining separate timestamps from GetTickCount() and > > QueryPerformanceCounter() time sources. > > Yes, and that is a prerequisite to have NowLoRes() that is pretty important > on some windows platform with slow QPC. Using GTC may be a prerequisite for NowLoRes(). There are alternatives to storing both timestamps in TimeStamp, but you may not like them :) > (In reply to Karl Tomlinson (needinfo?:karlt) from comment #0) > > I suspect an implementation that uses a single timestamp and periodically > > updates a difference for converting between the two sources, more similar to > > that prior to changes in bug 822490, would behave more as expected. May need > > to be careful about maintaining monotonicity of each of Now() and NowLoRes() > > (individually, not wrt each other), and finding another solution to the wake > > up issue described in bug 822490 comment 0. > > I'm strongly against this. That code was badly performing (slow), wasting > resources, using heavy locking, fixing one bug opened one or two others.. > no. I really don't want to do that step back. I could imagine locking produce performance issues. I don't know the issue with resource usage. Locking performance could be addressed by using thread-private conversion offsets. However, if we want to guarantee monotonicity between threads, then we'd need atomic variables or locking. I don't know the old code well enough to comment further. > I'd rather change the code to use QPC on stable TSC or > getSystemTimeAsFileTime when not. That would simplify stuff much more, no > calibration/fault detection at all. That may be a better solution. I don't know.