Hi Matt, On Fri, September 5, 2014 20:05, Matt Wilmas wrote: > Hi Anatol, > > > ----- Original Message ----- > From: "Anatol Belski" > Sent: Tuesday, September 02, 2014 > > >> 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: > > It looks like there can be "commit comments?" *shrug* In the future, > should a Pull Request usually be created? I didn't know if that sends > some sort of "Hey, add this!" notification, or simply makes it show up as > a PR. > >> - GetSystemTimeAdjustment can be disabled by some other app, so it >> should be called each time when gettimeofday() is called > > I don't know what you mean by "disabled." You mean the adjustment > changing? I sort of already alluded to not thinking it's necessary to > check it each time gettimeofday() is called -- I was already thinking of a > much more complicated method last year involving that stuff, before > finding this spring that Vista/7 aren't very finely adjustable like XP > regarding *setting* SystemTimeAdjustment... > > >> - GetSystemTimeAdjustment could fail, so return value should have be >> checked > > I know it could fail (though not sure why), technically, but I don't know > a reason for checking. time_update_rate is already initialized in time.c. > > >> - global variables should be moved into the globals struct >> > > Why? Just because? I thought it's fine to use true globals when it's > safe to (I've seen it in another place), like function pointers. No > threading issues when they're only modified during startup (1 thread). > >> 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. > > I was glad to see Stas' reply to "let's see for 5.4" to, well, see if > there's any other problems found. :-) There's that uniqid() bug/issue > like I said. > > > 5.4 is also the last "official" release (with PGO) for Windows XP. > coming to this right now. I'm just responding here as otherwise we'd have three parallel threads to follow, not handy.
I've tested your new variant and it doesn't show $t1 < $t0 on the same VM. However I still see an issue. The main issue that it tries to solve a hardware bug with math, that can work or not. Furthermore, looking closer at the math in the patch, I don't think it's correct. Obviously we neither should care about winxp at this times, nor about vista. Regarding win7, there are still too many issues to have a stable synchronizable timing in high resolution. You can agree or not, here are just the facts: - time-of-day clock accuracy depends on tick frequency (RTC interrupt) and/or additionally on HPET - QPC can be based on HPET, RDTSC, ACPI, etc. - the interrupt frequency depends on HW used - QPC is not synchronizable to an external source, while time-of-day clock is - QPC and partly time-of-day clock additionally depends whether it's a real HW or a VM, number of processors, thread safety, function call latency, etc. - while retrieving QPC is cheap, time-of-day clock will lead to loosing some microseconds - there are also some new HW which provides multimedia timing chips For more I'd just mention the site you already was linking to (did read that also a lot before), and especially to this doc http://www.windowstimestamp.com/PartIIAdjustmentofSystemTime.pdf starting at 4. As well as the MSDN pages for the corresponding functions. The consequence of all this is that it's completely unpredictable to mix QPC and time-of-day clock. There's no guarantee that time update interval will match the interrupt interval. Back to the math in the patch, lpTimeIncrement from GetSystemTimeAdjustment() will be constant, but lpTimeAdjustment can change. You currently don't take it in account. lpTimeAdjustment can be changed by a SetSystemTimeAdjustment() call, from an arbitrary source like some program or NTP update. Time adjustment can be disabled (and effectively that's not handled atm). That means from the patch ts = target - time_update_rate doesn't bring you to the time at the previous update interval. Say when lpTimeAdjustment == lpTimeIncrement, there were no gain at all (same as if time adjustment were disabled). That's why I meant GetSystemTimeAdjustment() should be called as frequently as gettimeofday(). From http://msdn.microsoft.com/en-us/library/windows/desktop/ms724394%28v=vs.85%29.aspx I read the following: ============= For each lpTimeIncrement period of time that actually passes, lpTimeAdjustment will be added to the time of day. If the lpTimeAdjustment value is smaller than lpTimeIncrement, the system time-of-day clock will advance at a rate slower than normal. If the lpTimeAdjustment value is larger than lpTimeIncrement, the time-of-day clock will advance at a rate faster than normal. If lpTimeAdjustment equals lpTimeIncrement, the time-of-day clock will advance at its normal speed. ============= The conclusion - while QPC is constant (or actually should be), the time-of-day clock might shift at an arbitrary rate. Taking in account, that the update interval can most likely be not the same (for instance RDTS vs PM clock), the result of the math is arbitrary. For more, GetSystemTimeAsFileTime() and GetSystemTimeAdjustment() will surely take several microseconds to complete, which add to the uncertainty. So even when used, they should be feched right after QPC. At the end, we have "some" value which passes into the range between the current QPC and the time-of-day clock values, but it's obviously not the value one could call even approximately correct. The uncertainty might be low enough to fit into the max resolution (say 300us were the max resolution and 30ms the uncertainty), however that's just a hope. For an application which really matters about the microsecond resolution it'll be not suitable. The timestamp will be just phony. That's what i meant with the example of the ebay bid, apart from net latency of course. But ebay uses the millisecond resolution anyway, like actually almost any real world app nowadays, still. It's more like example when you have PHP with this patch and try to communicate with another machine which is synchronizing time say per NTP and as in that case exact timing would matter, how much discrepancy will it have? After one month of running an FCGI process? And so on. IMHO how it looks with pre win8 - the time-of-day API is not suitable for performance measurements. I actually always presumed that it was your goal. For this goal the hrtime stuff can be used where you even can measure with pure QPC ticks when on a fast machine (say when you're out of even ns range). As if we cannot provide a valid synchronizable timestamp with QPC, the qualitative performance measurement is easy done using relative time intervals. May I suggest you to pack your work into a PECL extension, or maybe even merge with hrtime. As there are several aspects anyway where we have an issue, more or less. For example uniqid() you've mentioned. Or actually, usleep() IMHO - much more of issue than microtime currently as the resolution used there can be much worse. Anyway, it can come back to core after it was good tested, is well thought and solid. That's what I would suggest to do. Best regards Anatol -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
