Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

time.time() should be increasing #1358

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

s-cork
Copy link
Contributor

@s-cork s-cork commented Sep 5, 2021

it's possible to get negative timestamp differences in skulpt
But all timestamps should really be monotonically increasing
(unless the system time was changed between calls)

from time import time
t = time()
for _ in range(1000):
  if time() < t:
    print('back in time!')
  t = time()

This pr fixes that with an implementation that entirely depends on the browser performance api.
(falling back to the Date.now() api if the performance api is not available)
The previous implementation combined the two apis which led to negative timings since the fractional offset was not accurate with respect to Date.now().

I've added comments for browser compatibility links to mdn.
I haven't added additional tests since node doesn't support performance without a require('perf_hooks').


I also replace this with Sk.global in a few places which I think is safer.

@meredydd
Copy link
Contributor

meredydd commented Sep 6, 2021

Er...I'm not sure about this! On a few minutes' reading, it looks as though performance is a monotonic-clock API for performance measurements, not a date/time API, which means that using it like one invites weird effects (such as not updating on a system clock change, or weird readings over suspend/resume, and that's just the things I knew to Google off the top of my head).

@s-cork
Copy link
Contributor Author

s-cork commented Sep 6, 2021

The current skulpt implementation of time.time() already uses performance...
But it combines performance and Date in a way that causes negative time intervals.

I'm happy to just remove performance from time.time() and go with the Date api... But then it's a different pr.
thoughts @meredydd ?

@meredydd
Copy link
Contributor

meredydd commented Sep 6, 2021

I think relying entirely on Date makes sense for a datetime module. Let's do that instead.

Copy link
Contributor

@meredydd meredydd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per @s-cork, let's switch to using Date throughout

Copy link
Contributor

@meredydd meredydd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good now!

@meredydd meredydd merged commit 7b8687b into skulpt:master Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants