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.

Sign up for GitHub

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

Jump to bottom

Coarsen timeOrigin #106

Merged
merged 5 commits into from
Mar 9, 2021
Merged

Coarsen timeOrigin #106

merged 5 commits into from
Mar 9, 2021

Conversation

yoavweiss
Copy link
Contributor

@yoavweiss yoavweiss commented Feb 18, 2021 •
edited by pr-preview bot
Loading

This PR does multiple things:

  • It splits out timestamp coarsing into its own algorithm, as it's needed in multiple places
  • It coarsens the value returned "time origin timestamp" and hence from performance.timeOrigin
  • It changes the relative high resolution time definition to use "time origin timestamp" and hence to be coarsed, compared to the current algorithm which uses "time origin" directly (since "time origin" is a point in time, rather than a timestamp, it's unclear what the current diffing means, and "time origin timestamp" seems like a more appropriate value)

/cc @annevk

Preview | Diff

@yoavweiss yoavweiss requested a review from npm1 February 18, 2021 12:12
npm1
npm1 approved these changes Feb 19, 2021
View reviewed changes
Copy link
Contributor

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

lgtm

index.html Outdated
<div data-algorithm="relative high resolution time">
The <dfn data-export="">relative high resolution time</dfn> given a
{{DOMHighResTimeStamp}} |time| and a [=Realm/global object=] |global|,
runs the following steps:
<ul>
<li>Let |diff| be the difference between |time| and the |global|'s <a>time origin</a>.</li>
<li>Let |diff| be the difference between |time| and caliing [=time
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here, but also calling time origin timestamp sounds pretty weird so I suggest either modifying the name above or leaving this as it was.

@yoavweiss
Copy link
Contributor Author

@npm1 - PTAL?

npm1
npm1 reviewed Mar 9, 2021
View reviewed changes
index.html Outdated Show resolved Hide resolved
npm1
npm1 reviewed Mar 9, 2021
View reviewed changes
index.html Outdated Show resolved Hide resolved
index.html Outdated
<div data-algorithm="relative high resolution time">
The <dfn data-export="">relative high resolution time</dfn> given a
{{DOMHighResTimeStamp}} |time| and a [=Realm/global object=] |global|,
runs the following steps:
<ul>
<li>Let |diff| be the difference between |time| and the |global|'s <a>time origin</a>.</li>
<li>Let |diff| be the difference between |time| and calling [=get time
Copy link
Contributor

Choose a reason for hiding this comment

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

the result of calling? I'm not sure I see why adding 'get' makes this better, as the wording is now more awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified it, but open to change it to something else..

index.html
jitter |current time| such that its resolution will not exceed |time
resolution|.</li>
<li>Let |coarse time| be the result of calling [=coarsen time=] with
|current global| and |current time|.</li>
<li>Return the result of [=relative high resolution time=] given
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to coarsen the time in the relative high resolution time method instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, as it would make calling "relative HR time" safer.. Should we do that as a followup?

Copy link
Contributor

Choose a reason for hiding this comment

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

No big rush as currently it wasn't doing this, although isn't this just moving the line above to be called at the beginning of relative high resolution time or are there other changes I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easy, I just prefer not to mix it in this PR, as it's changing a different algorithm's semantics.

yoavweiss and others added 2 commits March 9, 2021 21:01
@yoavweiss @npm1
Suggestion
abf42cf 
Co-authored-by: npm1 <npm@chromium.org>
@yoavweiss
the result of calling
dbbe2ce
npm1
npm1 approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

lgtm

@yoavweiss yoavweiss merged commit 29866ae into w3c:gh-pages Mar 9, 2021
@yoavweiss
Copy link
Contributor Author

Thanks for reviewing! :)

@yoavweiss yoavweiss deleted the coarsen_time_origin branch March 9, 2021 20:46
@yoavweiss yoavweiss mentioned this pull request Mar 9, 2021
Merged
@yoavweiss yoavweiss mentioned this pull request Aug 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npm1 npm1 npm1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yoavweiss @npm1