Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:77034
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
Message-ID: <49fb94d5e1c8626544d7235492d9ae0b.squirrel@webmail.klapt.com>
In-Reply-To: <687BE274F5C24DE1A42E3484C7E52B6C@pc1>
References: <A85A75E7745344EDB50D308F48D2F2D5@pc1>
    <5404DBD1.8050604@sugarcrm.com>
    <687BE274F5C24DE1A42E3484C7E52B6C@pc1>
Date: Tue, 2 Sep 2014 13:43:55 +0200
To: "Matt Wilmas" <php_lists@realplain.com>
Cc: "Stas Malyshev" <smalyshev@sugarcrm.com>,
 internals@lists.php.net,
 "Pierre Joye" <pierre.php@gmail.com>,
 "Anatol Belski" <anatol.php@belski.net>
User-Agent: SquirrelMail/1.5.2 [SVN]
MIME-Version: 1.0
Content-Type: text/plain;charset=UTF-8
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Re: [INTERNALS-WIN] [PATCH] Microsecond resolution and
 accuracy on Windows
From: anatol.php@belski.net ("Anatol Belski")

On Mon, September 1, 2014 23:45, Matt Wilmas wrote:
> Hi Stas,
>
>
> ----- Original Message -----
> From: "Stas Malyshev"
> Sent: Monday, September 01, 2014
>
>
>> Hi!
>>
>>
>>> 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?)
>>
>> Looking at the patch, it looks like unfortunately it changes a global
>> structure (_php_win32_core_globals) which breaks binary compatibility. I
>>  think if you move the additional value to the end of the structure it
>> should be ok though, since other offsets should not change then.
>
> I was wondering about that myself (back when it was changed last, March
> 2013) when a few members were removed from that structure, so I figured it
>
>
> was OK to put one back. :-)  No problem with binary compatibility
> then...?
>
> http://git.php.net/?p=php-src.git;a=commitdiff;h=b903d2d6cdf9a9efac181a21
> e 95ea93dc8a864dd#patch5
>
>
>> I'm also not sure how important it is how have it for 5.4. Does the
>> problem that this patch fixes exist only in older versions of Windows or
>>  on all versions? What are the actual effects of this problem - is it
>> just lower resolution of microtime or there can be something seriously
>> wrong with the whole result? If it's just lower resolution, I'd prefer
>> this to go into 5.5 as the change is pretty extensive.
>
> I don't think the change is that extensive (maybe if you're comparing to
> current version ;-)).  As I said in my first "5.4 - last call" reply, it's
>
>
> similar to the previous old version.  You can see what was removed from
> time.c last March in the above diff...  My changes are like a simplified
> version of that (there for over a decade), restored, but without the
> possibility of "seriously wrong results" (not an issue now either, but
> we've also had very low resolution).
>
> The issues, and these changes, don't affect Windows 8/Server 2012 and
> later (they have a high-res time function like *nix gettimeofday()).  So
> Win 7
> and before.  See referenced bugs: #64633, #65626 (uniqid())
>
>
Unfortunately that's not a PR so I cannot comment there directly, so I'd
leave a couple of the comments to the code here:

- GetSystemTimeAdjustment can be disabled by some other app, so it should
be called each time when gettimeofday() is called
- GetSystemTimeAdjustment could fail, so return value should have be checked
- global variables should be moved into the globals struct

Matt, I really think it's too short in time to take this in. I would
suggest to leave this as is for 5.4 and 5.5 and stabilize to suggest to
5.6.1 or master after the good testing. It were probably applicable to
win7 then.

Thanks

Anatol