Bug 37743 - Separate clock time functions used by JavaScript from monotonically increasing time needed elsewhere
: Separate clock time functions used by JavaScript from monotonically increasin...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on: 61257
Blocks: 31256 62161 31550 58354 62159 62162 62175
  Show dependency treegraph
 
Reported: 2010-04-16 18:34 PDT by Yong Li
Modified: 2011-06-10 08:53 PDT (History)
24 users (show)

See Also:


Attachments
a start (2.29 KB, patch)
2010-04-16 18:49 PDT, Yong Li
barraclough: review-
Details | Formatted Diff | Diff
A bigger patch which rename the corresponding functions (87.10 KB, patch)
2010-05-03 22:25 PDT, Leo Yang
darin: review-
Details | Formatted Diff | Diff
A separated patch (14.84 KB, patch)
2010-05-05 19:12 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
A separated patch (JavaScript Core part) (15.31 KB, patch)
2010-05-05 22:08 PDT, Leo Yang
tkent: review-
Details | Formatted Diff | Diff
Patch part 1: base line (13.71 KB, patch)
2011-01-27 23:14 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Patch part 1: base line (14.43 KB, patch)
2011-01-28 01:48 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2011-05-31 15:30 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (4.49 KB, patch)
2011-05-31 16:14 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch based on Leo's (4.46 KB, patch)
2011-06-01 19:54 PDT, Jason Liu
jamesr: review-
Details | Formatted Diff | Diff
Patch based on Leo's (1.67 KB, patch)
2011-06-01 22:41 PDT, Jason Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2010-04-16 18:34:20 PDT
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.
Comment 1 Yong Li 2010-04-16 18:49:29 PDT
Created attachment 53588 [details]
a start

Can anyone add implementation of currentUTCTime() and fix currentTime() for his favorite platform? Thanks
Comment 2 WebKit Review Bot 2010-04-16 18:50:25 PDT
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.
Comment 3 WebKit Review Bot 2010-04-16 19:10:20 PDT
Attachment 53588 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1671373
Comment 4 Yong Li 2010-04-16 19:13:22 PDT
(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 5 Gavin Barraclough 2010-04-17 14:05:39 PDT
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.
Comment 6 Dmitry Titov 2010-04-19 17:57:48 PDT
(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.
Comment 7 Dmitry Titov 2010-04-19 18:29:08 PDT
(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.
Comment 8 George Staikos 2010-04-21 23:14:41 PDT
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.
Comment 9 Yong Li 2010-04-22 12:49:56 PDT
(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?)
Comment 10 Leo Yang 2010-05-03 22:25:23 PDT
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.
Comment 11 WebKit Review Bot 2010-05-03 22:30:04 PDT
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.
Comment 12 Leo Yang 2010-05-03 22:33:23 PDT
(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.
Comment 13 WebKit Review Bot 2010-05-03 23:25:10 PDT
Attachment 54996 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2123007
Comment 14 WebKit Review Bot 2010-05-04 00:30:18 PDT
Attachment 54996 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2075008
Comment 15 Darin Adler 2010-05-04 10:19:36 PDT
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.
Comment 16 Leo Yang 2010-05-04 18:35:47 PDT
(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?
Comment 17 Leo Yang 2010-05-05 19:12:47 PDT
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.
Comment 18 WebKit Review Bot 2010-05-05 19:19:28 PDT
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.
Comment 19 WebKit Review Bot 2010-05-05 20:33:49 PDT
Attachment 55193 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2191006
Comment 20 Leo Yang 2010-05-05 22:08:47 PDT
Created attachment 55205 [details]
A separated patch (JavaScript Core part)
Comment 21 WebKit Review Bot 2010-05-05 22:09:45 PDT
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.
Comment 22 WebKit Review Bot 2010-05-05 23:24:37 PDT
Attachment 55205 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2192014
Comment 23 WebKit Review Bot 2010-05-09 14:10:42 PDT
Attachment 55205 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2191088
Comment 24 Kent Tamura 2010-06-04 01:42:55 PDT
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.
Comment 25 Sam Weinig 2010-06-04 10:56:49 PDT
(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?
Comment 26 Leo Yang 2011-01-27 23:14:27 PST
Created attachment 80421 [details]
Patch part 1: base line
Comment 27 Build Bot 2011-01-28 00:12:00 PST
Attachment 80421 [details] did not build on win:
Build output: http://queues.webkit.org/results/7545380
Comment 28 Leo Yang 2011-01-28 01:48:38 PST
Created attachment 80427 [details]
Patch part 1: base line

Fixed build failure on Windows.
Comment 29 Antonio Gomes 2011-05-19 06:34:29 PDT
Progresses here, leo?
Comment 30 Leo Yang 2011-05-22 20:17:55 PDT
(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.
Comment 31 Darin Adler 2011-05-31 15:04:39 PDT
*** Bug 60936 has been marked as a duplicate of this bug. ***
Comment 32 Darin Adler 2011-05-31 15:05:32 PDT
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.
Comment 33 James Simonsen 2011-05-31 15:28:26 PDT
(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.
Comment 34 James Simonsen 2011-05-31 15:30:42 PDT
Created attachment 95495 [details]
Patch
Comment 35 Darin Adler 2011-05-31 15:33:37 PDT
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?
Comment 36 Darin Adler 2011-05-31 15:34:34 PDT
While I don’t want to rename the existing function to currentUTCTime, I was thinking that clockTime might be a good name for it.
Comment 37 James Simonsen 2011-05-31 15:38:06 PDT
(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.
Comment 38 Darin Adler 2011-05-31 15:41:32 PDT
(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.
Comment 39 James Simonsen 2011-05-31 15:44:11 PDT
(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...
Comment 40 James Simonsen 2011-05-31 16:14:30 PDT
Created attachment 95502 [details]
Patch
Comment 41 Igor Trindade Oliveira 2011-05-31 17:57:55 PDT
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
Comment 42 James Simonsen 2011-05-31 18:12:02 PDT
(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.
Comment 43 Jason Liu 2011-06-01 19:54:54 PDT
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 44 James Robinson 2011-06-01 19:59:31 PDT
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!
Comment 45 Jason Liu 2011-06-01 20:06:06 PDT
(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?
Comment 46 Jason Liu 2011-06-01 20:07:39 PDT
(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.
Comment 47 Jason Liu 2011-06-01 22:41:10 PDT
Created attachment 95723 [details]
Patch based on Leo's

Add monotonicallyIncreasingTime() for Linux
Comment 48 James Simonsen 2011-06-06 14:13:24 PDT
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 49 Igor Trindade Oliveira 2011-06-06 14:17:01 PDT
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 50 James Robinson 2011-06-06 14:38:56 PDT
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 51 WebKit Review Bot 2011-06-06 16:05:10 PDT
Comment on attachment 95502 [details]
Patch

Clearing flags on attachment: 95502

Committed r88199: <http://trac.webkit.org/changeset/88199>
Comment 52 WebKit Review Bot 2011-06-06 16:05:19 PDT
All reviewed patches have been landed.  Closing bug.