Bug 37743 – Separate clock time functions used by JavaScript from monotonically increasing time needed elsewhere

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yong Li]

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.

[Yong Li]

Created attachment 53588 [details]
a start

Can anyone add implementation of currentUTCTime() and fix currentTime() for his favorite platform? Thanks

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 53588 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1671373

[Yong Li]

(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?

[Gavin Barraclough]

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.

[Dmitry Titov]

(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.

[Dmitry Titov]

(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.

[George Staikos]

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.

[Yong Li]

(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?)

[Leo Yang]

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.

[WebKit Review Bot]

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.

[Leo Yang]

(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.

[WebKit Review Bot]

Attachment 54996 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2123007

[WebKit Review Bot]

Attachment 54996 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2075008

[Darin Adler]

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.

[Leo Yang]

(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?

[Leo Yang]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 55193 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2191006

[Leo Yang]

Created attachment 55205 [details]
A separated patch (JavaScript Core part)

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 55205 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2192014

[WebKit Review Bot]

Attachment 55205 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2191088

[Kent Tamura]

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.

[Sam Weinig]

(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?

[Leo Yang]

Created attachment 80421 [details]
Patch part 1: base line

[Build Bot]

Attachment 80421 [details] did not build on win:
Build output: http://queues.webkit.org/results/7545380

[Leo Yang]

Created attachment 80427 [details]
Patch part 1: base line

Fixed build failure on Windows.

[Antonio Gomes]

Progresses here, leo?

[Leo Yang]

(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.

[Darin Adler]

*** Bug 60936 has been marked as a duplicate of this bug. ***

[Darin Adler]

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.

[James Simonsen]

(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.

[James Simonsen]

Created attachment 95495 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

While I don’t want to rename the existing function to currentUTCTime, I was thinking that clockTime might be a good name for it.

[James Simonsen]

(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.

[Darin Adler]

(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.

[James Simonsen]

(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...

[James Simonsen]

Created attachment 95502 [details]
Patch

[Igor Trindade Oliveira]

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

[James Simonsen]

(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.

[Jason Liu]

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.

[James Robinson]

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!

[Jason Liu]

(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?

[Jason Liu]

(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.

[Jason Liu]

Created attachment 95723 [details]
Patch based on Leo's

Add monotonicallyIncreasingTime() for Linux

[James Simonsen]

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

[Igor Trindade Oliveira]

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

[James Robinson]

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?

[WebKit Review Bot]

Comment on attachment 95502 [details]
Patch

Clearing flags on attachment: 95502

Committed r88199: <http://trac.webkit.org/changeset/88199>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.