The only case currentTime() is used as current UTC time is in by jsCurrentTime() (through currentTimeMS()). In all other cases, currentTime() is used as a system tick count and is supposed to be monotonically increasing, in other words, it shouldn't come backwards. These cases include (but not limited to): 1) WebCore 2) Threading code 3) TimeoutChecker We should separate these 2 usages by removing the dependency of jsCurrentTime() on currentTime(). Then we can make jsCurrentTime() return current UTC time, and make currentTime() an increasing system count.
Created attachment 53588 [details] a start Can anyone add implementation of currentUTCTime() and fix currentTime() for his favorite platform? Thanks
Attachment 53588 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 JavaScriptCore/wtf/CurrentTime.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 53588 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1671373
(In reply to comment #3) > Attachment 53588 [details] did not build on chromium: > Build output: http://webkit-commit-queue.appspot.com/results/1671373 Does chromium use another CurrentTime.h? Or it needs a clean build?
Comment on attachment 53588 [details] a start This looks like a good fix, but I'm a little concerned about the existing function continuing to be called 'currentTime', since it does not return the current time! 'tickCount()', perhaps, or 'monotonicTime()'? I'd suggest having caught this bug it'd be really good to change the name of that function (plus 'currentTimeMS' to match), to make its meaning a little clearer & prevent similar problems in the future. r- to this patch for chromium breakage.
(In reply to comment #5) > (From update of attachment 53588 [details]) > This looks like a good fix, but I'm a little concerned about the existing > function continuing to be called 'currentTime', since it does not return the > current time! 'tickCount()', perhaps, or 'monotonicTime()'? I'd suggest > having caught this bug it'd be really good to change the name of that function > (plus 'currentTimeMS' to match), to make its meaning a little clearer & prevent > similar problems in the future. I'm a bit puzzled. All the patch does is to add a synonym for currentTime, calling it also currentUTCTime... They both now exist and return exactly the same thing. Looking at the resulting code, it's not clear what is going on, and unless the implementation for all platforms is actually coming in the next patch, it creates confusing code. It seems the better alternative would be to have a notion of time intervals rather then absolute time for things like animations etc. Something like a stopwatch. Then there is no question of what kind of time it is or whether or not it changes based on user changing the time in the system. Something like: Stopwatch stopwatch; stopwatch.start(); // starts ticking at 0 ... stopwatch.getTime() // returns current time ... stopwatch.stop(); // stops advancing stopwatch.reset(); // drops to 0 again I could imagine currentTime to just continue to exist as today (no renaming necessary), since it is actually returning UTC time, and perhaps gradually switching animations etc to a non-absolute time interval source like a stopwatch.
(In reply to comment #6) > > It seems the better alternative would be to have a notion of time intervals > rather then absolute time for things like animations etc. Something like a > stopwatch. Then there is no question of what kind of time it is or whether or > not it changes based on user changing the time in the system. Something like: > > Stopwatch stopwatch; > stopwatch.start(); // starts ticking at 0 > ... Never mind, it actually could make things like sorting Timers slower and make it difficult to synchronize multiple intervals. Having absolute timer makes a lot of things simpler. Sorry for noise. Perhaps a good name for 'monotonic time' function is all what's needed.
This causes a regression in the Date constructor, for which I have a fix and sent to Yong. I think the Date constructor is actually not quite right.
(In reply to comment #8) > This causes a regression in the Date constructor, for which I have a fix and > sent to Yong. I think the Date constructor is actually not quite right. Probably jsCurrentTime should take the "exec" pointer so it can use the cached UTC offset and cached DST offset (as it's supposed to?)
Created attachment 54996 [details] A bigger patch which rename the corresponding functions Rename currentTime() to currentUTCTime(), currentTimeMS() to currentUTCTimeMS(); add monotonicTime() Implement monotonicTime() in posix based system and redirect monotonicTime() implementation to currentUTCTime() in non-posix implmentation.
Attachment 54996 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/CurrentTime.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 81 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Attachment 54996 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" > exit_code: 1 > JavaScriptCore/wtf/CurrentTime.h:42: Code inside a namespace should not be > indented. [whitespace/indent] [4] > Total errors found: 1 in 81 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. This style issue is derived from the existing code.
Attachment 54996 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2123007
Attachment 54996 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/2075008
Comment on attachment 54996 [details] A bigger patch which rename the corresponding functions This seems to break the build on every platform. Maybe you could break this up into multiple patches. One could change the calls at all the call sites and so be entirely mechanical. The part of this that needs careful review is that we have to figure out at each call site if it's OK to use the monotonic time rather than the clock time. It would be good to review those carefully a few at a time instead of in a single giant patch. Perhaps the first patch would be to convert all the call sites where it's very clear and completely obvious which to use.
(In reply to comment #15) > (From update of attachment 54996 [details]) > This seems to break the build on every platform. > > Maybe you could break this up into multiple patches. One could change the calls > at all the call sites and so be entirely mechanical. > > The part of this that needs careful review is that we have to figure out at > each call site if it's OK to use the monotonic time rather than the clock time. > It would be good to review those carefully a few at a time instead of in a > single giant patch. Perhaps the first patch would be to convert all the call > sites where it's very clear and completely obvious which to use. agree! How to break to multiple patches? Perhaps I would keep currentTime() & currentTimeMS() and add monotonicTime() & currentUTCTime() & currentUTCTimeMS(), then change some call sites and submit a patch, and then change some other call sites and submit a patch, ...., in the last patch, remove currentTime() & currentTimeMS(). Would it be ok?
Created attachment 55193 [details] A separated patch keep the original interfaces to separate the previous patch to multiple patches. This is the first patch to change call sites in JavaScriptCore.
Attachment 55193 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/CurrentTime.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 55193 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2191006
Created attachment 55205 [details] A separated patch (JavaScript Core part)
Attachment 55205 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/CurrentTime.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 55205 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2192014
Attachment 55205 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/2191088
Comment on attachment 55205 [details] A separated patch (JavaScript Core part) currentTime()-related implementation of Chromium should be put to WebKit/chromium/src/ChromiumCurrentTime.cpp. As for Windows, I guess the error was no exported symbols for currentUTCTime() and monotonicTime(). JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def should have their symbols.
(In reply to comment #24) > (From update of attachment 55205 [details]) > currentTime()-related implementation of Chromium should be put to WebKit/chromium/src/ChromiumCurrentTime.cpp. This sounds like a layering violation?
Created attachment 80421 [details] Patch part 1: base line
Attachment 80421 [details] did not build on win: Build output: http://queues.webkit.org/results/7545380
Created attachment 80427 [details] Patch part 1: base line Fixed build failure on Windows.
Progresses here, leo?
(In reply to comment #29) > Progresses here, leo? I'm preparing a patch which renames currentTime to currentUTCTime and currentTimeMS to currentUTCTimeMS. And then will move to the next step after the patch goes in.
*** Bug 60936 has been marked as a duplicate of this bug. ***
Personally, I like the approach in bug 60936, where we kept the existing function name for clock time and used a new function name for monotonically increasing time, better than the approach in the pathces here where we change currentTime to mean monotonically increasing time.
(In reply to comment #32) > Personally, I like the approach in bug 60936, where we kept the existing function name for clock time and used a new function name for monotonically increasing time, better than the approach in the pathces here where we change currentTime to mean monotonically increasing time. Yeah, I'd rather just add the new function and move callers over on a case-by-case basis. We'll also need that lead time to implement monotonic time on all the platforms. I'll resubmit my patch here, since that's the path it was on. We should add other bugs to track the callers we want to migrate (like timers, for starters) and have them depend on this. This bug seems to be trying to do too much.
Created attachment 95495 [details] Patch
Comment on attachment 95495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95495&action=review > Source/JavaScriptCore/wtf/CurrentTime.h:63 > +uint64_t currentTickCount(); It's a little awkward to use a 64-bit integer containing microseconds. What’s the argument against using a double containing seconds for this? Can we name this monotonicallyIncreasingTim instead of currentTickCount?
While I don’t want to rename the existing function to currentUTCTime, I was thinking that clockTime might be a good name for it.
(In reply to comment #35) > (From update of attachment 95495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95495&action=review > > > Source/JavaScriptCore/wtf/CurrentTime.h:63 > > +uint64_t currentTickCount(); > > It's a little awkward to use a 64-bit integer containing microseconds. What’s the argument against using a double containing seconds for this? The hope was that having separate types would discourage people from confusing them with the values returned from currentTime. A double of seconds is just as useful for my purposes though, so I'm happy to switch if you'd like. > Can we name this monotonicallyIncreasingTim instead of currentTickCount? Sure, I will upload a patch shortly.
(In reply to comment #37) > The hope was that having separate types would discourage people from confusing them with the values returned from currentTime. Interesting and clever. Not sure I like it, though. It’s pretty easy to mix integers and floating point numbers by accident anyway. > A double of seconds is just as useful for my purposes though, so I'm happy to switch if you'd like. I slightly prefer it.
(In reply to comment #38) > (In reply to comment #37) > > The hope was that having separate types would discourage people from confusing them with the values returned from currentTime. > > Interesting and clever. Not sure I like it, though. It’s pretty easy to mix integers and floating point numbers by accident anyway. Agreed. We'd need a class to do it properly and that seems heavy for what we're trying to accomplish. > > A double of seconds is just as useful for my purposes though, so I'm happy to switch if you'd like. > > I slightly prefer it. Patch on the way...
Created attachment 95502 [details] Patch
This patch is just covering MacOSX right? and other plataforms like Windows and Unix? (In reply to comment #40) > Created an attachment (id=95502) [details] > Patch
(In reply to comment #41) > This patch is just covering MacOSX right? and other plataforms like Windows and Unix? Yep. If we land this, I'll file bugs against the other platforms and fix a few of them myself. If a platform is missing, we'll just fall back to the dumb default implementation.
Created attachment 95706 [details] Patch based on Leo's Add currentUTCTime() which should be used by jsCurrentTime() as clock time.Change currentTime() with monotonically increasing codes. And this patch can fix Bug21256 which has been tested at Qt version on Ubuntu. Bug 31256 - SVG Animation freezes when system time was changed during playback. And we can make these function's name be more rational with Bug 61257.
Comment on attachment 95706 [details] Patch based on Leo's View in context: https://bugs.webkit.org/attachment.cgi?id=95706&action=review I don't think you can change the semantics of javascript Date.now()! Have you followed the recent discussion on this bug? > Source/JavaScriptCore/wtf/CurrentTime.cpp:317 > +// Added currentUTCTime which should be used by JScurrentTime(). And changed currentTime() with > +// monotonically Increasing codes. woah, you can't do this!
(In reply to comment #44) > (From update of attachment 95706 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95706&action=review > > I don't think you can change the semantics of javascript Date.now()! > > Have you followed the recent discussion on this bug? > > > Source/JavaScriptCore/wtf/CurrentTime.cpp:317 > > +// Added currentUTCTime which should be used by JScurrentTime(). And changed currentTime() with > > +// monotonically Increasing codes. > > woah, you can't do this! Hi,Would you pls tell me how I change the semantics?
(In reply to comment #43) > Created an attachment (id=95706) [details] > Patch based on Leo's > > Add currentUTCTime() which should be used by jsCurrentTime() as clock > time.Change currentTime() with monotonically increasing codes. > And this patch can fix Bug21256 which has been tested at Qt version on Ubuntu. > Bug 31256 - SVG Animation freezes when system time was changed during playback. > And we can make these function's name be more rational with Bug 61257. Sorry, this patch can fix Bug31256 not Bug21256.
Created attachment 95723 [details] Patch based on Leo's Add monotonicallyIncreasingTime() for Linux
The discussion seems to have died out on both bugs. Assuming everyone is happy now, can someone r+ this now so we can start landing implementations and eventually switch over to using it? This adds the function, a dumb default implementation, and a Mac implementation: https://bugs.webkit.org/attachment.cgi?id=95502&action=review This adds a Linux implementation: https://bugs.webkit.org/attachment.cgi?id=95723&action=review
For Linux/Unix i think is better to implement using clock_gettime. It is better documented than times(0). And clock_gettime is a POSIX.1-2001. (In reply to comment #48) > The discussion seems to have died out on both bugs. Assuming everyone is happy now, can someone r+ this now so we can start landing implementations and eventually switch over to using it? > > This adds the function, a dumb default implementation, and a Mac implementation: > > https://bugs.webkit.org/attachment.cgi?id=95502&action=review > > This adds a Linux implementation: > > https://bugs.webkit.org/attachment.cgi?id=95723&action=review
Comment on attachment 95502 [details] Patch R=me, this looks like a good start. Can you file bugs on the platforms other than PLATFORM(MAC) to implement a higher-quality version of this if they choose to?
Comment on attachment 95502 [details] Patch Clearing flags on attachment: 95502 Committed r88199: <http://trac.webkit.org/changeset/88199>
All reviewed patches have been landed. Closing bug.