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
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 61257
: 31256 31550 58354 62159 62161 62162 62175
  Show dependency treegraph
 
Reported: 2010-04-16 18:34 PST by
Modified: 2011-06-10 08:53 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-16 18:34:20 PST
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 From 2010-04-16 18:49:29 PST -------
Created an attachment (id=53588) [details]
a start

Can anyone add implementation of currentUTCTime() and fix currentTime() for his favorite platform? Thanks
------- Comment #2 From 2010-04-16 18:50:25 PST -------
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 From 2010-04-16 19:10:20 PST -------
Attachment 53588 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1671373
------- Comment #4 From 2010-04-16 19:13:22 PST -------
(In reply to comment #3)
> Attachment 53588 [details] [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 From 2010-04-17 14:05:39 PST -------
(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.

r- to this patch for chromium breakage.
------- Comment #6 From 2010-04-19 17:57:48 PST -------
(In reply to comment #5)
> (From update of attachment 53588 [details] [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 From 2010-04-19 18:29:08 PST -------
(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 From 2010-04-21 23:14:41 PST -------
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 From 2010-04-22 12:49:56 PST -------
(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 From 2010-05-03 22:25:23 PST -------
Created an attachment (id=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 From 2010-05-03 22:30:04 PST -------
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 From 2010-05-03 22:33:23 PST -------
(In reply to comment #11)
> Attachment 54996 [details] [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 From 2010-05-03 23:25:10 PST -------
Attachment 54996 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2123007
------- Comment #14 From 2010-05-04 00:30:18 PST -------
Attachment 54996 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2075008
------- Comment #15 From 2010-05-04 10:19:36 PST -------
(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.
------- Comment #16 From 2010-05-04 18:35:47 PST -------
(In reply to comment #15)
> (From update of attachment 54996 [details] [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 From 2010-05-05 19:12:47 PST -------
Created an attachment (id=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 From 2010-05-05 19:19:28 PST -------
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 From 2010-05-05 20:33:49 PST -------
Attachment 55193 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2191006
------- Comment #20 From 2010-05-05 22:08:47 PST -------
Created an attachment (id=55205) [details]
A separated patch (JavaScript Core part)
------- Comment #21 From 2010-05-05 22:09:45 PST -------
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 From 2010-05-05 23:24:37 PST -------
Attachment 55205 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2192014
------- Comment #23 From 2010-05-09 14:10:42 PST -------
Attachment 55205 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2191088
------- Comment #24 From 2010-06-04 01:42:55 PST -------
(From update of attachment 55205 [details])
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 From 2010-06-04 10:56:49 PST -------
(In reply to comment #24)
> (From update of attachment 55205 [details] [details])
> currentTime()-related implementation of Chromium should be put to WebKit/chromium/src/ChromiumCurrentTime.cpp.

This sounds like a layering violation?
------- Comment #26 From 2011-01-27 23:14:27 PST -------
Created an attachment (id=80421) [details]
Patch part 1: base line
------- Comment #27 From 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 From 2011-01-28 01:48:38 PST -------
Created an attachment (id=80427) [details]
Patch part 1: base line

Fixed build failure on Windows.
------- Comment #29 From 2011-05-19 06:34:29 PST -------
Progresses here, leo?
------- Comment #30 From 2011-05-22 20:17:55 PST -------
(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 From 2011-05-31 15:04:39 PST -------
*** Bug 60936 has been marked as a duplicate of this bug. ***
------- Comment #32 From 2011-05-31 15:05:32 PST -------
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 From 2011-05-31 15:28:26 PST -------
(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 From 2011-05-31 15:30:42 PST -------
Created an attachment (id=95495) [details]
Patch
------- Comment #35 From 2011-05-31 15:33:37 PST -------
(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?

Can we name this monotonicallyIncreasingTim instead of currentTickCount?
------- Comment #36 From 2011-05-31 15:34:34 PST -------
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 From 2011-05-31 15:38:06 PST -------
(In reply to comment #35)
> (From update of attachment 95495 [details] [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 From 2011-05-31 15:41:32 PST -------
(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 From 2011-05-31 15:44:11 PST -------
(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 From 2011-05-31 16:14:30 PST -------
Created an attachment (id=95502) [details]
Patch
------- Comment #41 From 2011-05-31 17:57:55 PST -------
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] [details]
> Patch
------- Comment #42 From 2011-05-31 18:12:02 PST -------
(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 From 2011-06-01 19:54:54 PST -------
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.
------- Comment #44 From 2011-06-01 19:59:31 PST -------
(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!
------- Comment #45 From 2011-06-01 20:06:06 PST -------
(In reply to comment #44)
> (From update of attachment 95706 [details] [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 From 2011-06-01 20:07:39 PST -------
(In reply to comment #43)
> Created an attachment (id=95706) [details] [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 From 2011-06-01 22:41:10 PST -------
Created an attachment (id=95723) [details]
Patch based on Leo's

Add monotonicallyIncreasingTime() for Linux
------- Comment #48 From 2011-06-06 14:13:24 PST -------
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 From 2011-06-06 14:17:01 PST -------
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 From 2011-06-06 14:38:56 PST -------
(From update of attachment 95502 [details])
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 From 2011-06-06 16:05:10 PST -------
(From update of attachment 95502 [details])
Clearing flags on attachment: 95502

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