Re: [PATCH] Microsecond resolution and accuracy on Windows

Hi Anatol!

----- Original Message -----
From: "Anatol Belski"
Sent: Tuesday, September 02, 2014

> Hi Matt,
>
> On Mon, September 1, 2014 20:36, Matt Wilmas wrote:
>> Hi all!
>>
>>
>> I'm back after several years, and will have a few more changes for
>> Windows,
>> at least.  (It was CVS back then, so I still have to figure some things
>> out...  Just had to edit files on Github site. :-/)
>>
>> Anyway, this patch is for microtime, etc. on Windows XP-7.  In March
>> 2013,
>>
>>
>> the fix for bug #64370 reduced the resolution to 1/64 or 1/100 of a
>> second
>>
>> or so -- hardly "micro" time. :*(  See also bugs #64633 and #65626.  The
>> highest resolution time function available before Windows 8 has simply
>> been used by itself since then.  High-resolution Performance Counters 
>> were
>>  always used until then, but in a way that may become inaccurate and
>> error-prone on some Windows+(virtual) hardware combinations.
>>
>> One problem with the long-standing previous Performance Counter
>> implementation is that it tried to be monotonic (always increasing).  But
>> gettimeofday() IS supposed to be affected by changes in system time.
>> Another problem was assuming that Performance Counters would track at a
>> nearly-perfect rate with the real clock even without system time changes.
>>
>> The solution to have high-resolution like the past, and accuracy like now
>>  (after #64370 fix) is fairly simple and straightforward: Use Performance
>>  Counters, but check that the value is within "range" of the real,
>> current time!
>>
>> With this implementation, you get high-resolution at least over short
>> periods (e.g. when it matters) AND accuracy over a longer time between
>> calls (e.g. won't notice few milliseconds of lost resolution), with no
>> buggy/incorrect times.  (It seems there *could* be a Counter
>> synchronization issue between multiple cores/processors on *some* 
>> hardware
>> on *XP*.  I don't think this is a concern, nor related to previous bugs?
>> http://msdn.microsoft.com/en-us/library/windows/desktop/dn553408(v=vs.85)
>> .
>> aspx#qpc_support_in_windows_versions )
>>
>> It's much more optimized than what's there now, and slightly over the old
>>  implementation.  Not sure if I should give the saved patch link, or the
>> "live compare" (?) on Github, so I'll do both for now:
>> http://realplain.com/php/microtime_5_4.diff
>> https://github.com/matt-moo/php-src/compare/PHP-5.4.diff
>>
>>
>> Against 5.4 since that's what I quickly worked on so it's ready for the
>> next 5.4 release (Stas?). (Although I guess we're supposed to change the
>> oldest
>>
>> branch usually?)
>>
>> What do you think?  Questions or comments?
>>
>>
> unfortunately the patch doesn't fix the both issues, a quick test shows
> just a return to the old behaviour:
>
>>php -n -r "$t0 = microtime(true); do { $t1 = microtime(true); echo $t1 -
> $t0, PHP_EOL; } while($t1 >= $t0 && ($t0 = $t1));"
> ..........................................
> 4.7922134399414E-5
> 4.6968460083008E-5
> 4.7922134399414E-5
> 4.8160552978516E-5
> 4.7922134399414E-5
> 4.7922134399414E-5
> 4.8160552978516E-5
> 4.887580871582E-5
> 0.00020909309387207
> -0.015614986419678
>
> I took an arbitrary win7 VM for that. The error happens after a couple of
> seconds of running. That's just before starting to argue. We probably
> would get WTFs from the users which had this before and had that fixed.
> IMHO, it's still better not to have $t1 < $t0 even with worse accuracy.

Thanks for all the feedback!  I was just waiting to get something "solid" 
before replying. :-)

I was like, "Well, hell," when I saw this first reply. :*(  BUT, I think 
it's fixed now!  Patch updated (at links above for anyone).

I just quickly implemented what I had in my mind for a while Sat. morning 
(half the time was trying to consider what would happen in different 
scenarios), so I didn't spend much time on it at all. :-)  Brief checking 
after, but yep, I was able to reproduce your result (XP, real system) after 
a few minutes (15-20 max) and realized what was happening.

I thought about and tried a few things in the following couple days, before 
settling (more "obviously" now) on what's there now ("computed" 
out-of-valid-range logic was too simple before, oops).  Maybe THIS is 
actually the correct logic I had in mind last October!

I haven't seen any problem with it after hours and hours (which seems 
right).  The only time there could be t1 < t0 is if the clock is set 
backwards (which obviously happens now with the current low-res version).

> I still think that this issue is unsolvable within PHP. Fixing this for
> one group of users will break it for another. There are too many factors
> affecting this. Besides the hardware we cannot affect, there are programs
> affecting the behaviour (for example some could use
> SetSystemTimeAdjustment()).

See if you can break/find a problem with it now, please. :-)

> And that is IMO the main mistake with this approach (and the one before
> removing QPC) - it's that QPC is based on an independent source, while
> GetSystemTimeAdjustment and similar time functions are synchronizable to
> an external source. Haven't tried, but just asking - would it work correct
> on day light saving days? QPC doc also states it is dedicated for time
> intervals measurements. The user would think that he gets a real correct
> microtime timestamp, while in reality it's a "computed" one, but the
> equation used for the computation is far incomplete.

By "it," you mean my/any QPC implementation with DST changes?  Yes, 
GetSystemTime... is like time() I assume and doesn't change with DST --  
otherwise PHP's date stuff would be affected.

And my version of course always respects the currently-reported time, 
anyway, unlike the OLD version.

> Inbetween i was developing pecl.php.net/hrtime which uses APIs similar to
> QPC on linux. While testing it even on linux we have seen a shift between
> gettimeofday and monotonic clocks. Well, there one needs much longer time
> to reproduce and in reality one doesn't know which of those APIs exactly
> has a bug. But just as an example.

Yeah, I saw about your hrtime package in the one bug comment (interesting), 
but haven't looked at it yet...

> At the end - I would not include this patch that fast especially shortly
> before the PHP-5.4 doors are closing afterwards. While it were of course
> amazing to have a fix, the patch doesn't seem to fix the bad accuracy but
> brings the old behavior back. I have also a couple of notes to the code,
> i'd leave my comments on the patch repo. Matt, I have anyway to thank you
> for the effort and energy you spent on this.

Still hoping for 5.4 for at least the "security" related view with uniqid(). 
:-)

BTW, I also removed the useless modulo from microtime().  (The GitHub patch 
is breaking the uniqid.c header... ignore.)

> Regards
>
> Anatol

Thanks again,
Matt