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

Coarsen the granularity of timing functions exposed to DOM #25656

Closed
pshaughn opened this issue Jan 30, 2020 · 3 comments
Closed

Coarsen the granularity of timing functions exposed to DOM #25656

pshaughn opened this issue Jan 30, 2020 · 3 comments
Assignees
Labels

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Jan 30, 2020

https://www.w3.org/TR/hr-time-2/#clock-resolution

#25296 and #21276 are because we're not doing this, so the apparent granularity depends on randomly how far apart the consecutive performance.now() calls are.

@pshaughn pshaughn self-assigned this Jan 30, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 30, 2020

Those two issues are about performance.now(), but navigation-timing/test_timing_attributes_order.html does similarly for attributes like fetchStart. grepping precise_time_ns in Servo's code gets quite a few hits, and I'm guessing many of them are DOM-exposed.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 31, 2020

Timestamps are frequently sent through constellation messages and I don't have my head around where they end up.

It seems to me that uses of precise_time_ns in devtools, gfx, layout_thread, profile, and profile_traits don't end up anywhere DOM-exposed but I'm not completely sure.

I'm pretty sure that compositor, net, net_traits, script, and script_traits have at least some of their precise_time_ns usages DOM-exposed, but not necessarily all.

I do not understand metrics/lib.rs.

If someone could give me a list of which values can end up DOM-exposed via what objects, then I could probably do the rest from there.

@pshaughn pshaughn removed their assignment Jan 31, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 31, 2020

It occurs to me I might have overthought this; we really only need to coarsen at the DOM boundary where the user's going to see it, and that's just every method that returns a DOMHighResTimeStamp

@pshaughn pshaughn self-assigned this Jan 31, 2020
@CYBAI CYBAI added the A-content/dom label Jan 31, 2020
@pshaughn pshaughn mentioned this issue Jan 31, 2020
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Feb 3, 2020
Expose DOMHighResTimeStamps at lower res

As explained in https://www.w3.org/TR/hr-time-2/#clock-resolution and tested in a few WPT tests, we're not supposed to show Javascript the full resolution of OS timestamps. This fixes that on all the DOMHighResTimeStamp interfaces that weren't already limiting themselves to integer milliseconds.

The specific choice of how to coarsen the resolution is extremely bikesheddable; I commented the reasoning for my arbitrary choice but I admit it's arbitrary.

A couple tests of timing resolution still fail, but because they're seeing undefined and NaN values due to unimplemented attributes (#25658).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25656 fix #25296 fix #21276

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 3, 2020
Expose DOMHighResTimeStamps at lower res

As explained in https://www.w3.org/TR/hr-time-2/#clock-resolution and tested in a few WPT tests, we're not supposed to show Javascript the full resolution of OS timestamps. This fixes that on all the DOMHighResTimeStamp interfaces that weren't already limiting themselves to integer milliseconds.

The specific choice of how to coarsen the resolution is extremely bikesheddable; I commented the reasoning for my arbitrary choice but I admit it's arbitrary.

A couple tests of timing resolution still fail, but because they're seeing undefined and NaN values due to unimplemented attributes (#25658).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25656 fix #25296 fix #21276

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 10, 2020
Expose DOMHighResTimeStamps at lower res

As explained in https://www.w3.org/TR/hr-time-2/#clock-resolution and tested in a few WPT tests, we're not supposed to show Javascript the full resolution of OS timestamps. This fixes that on all the DOMHighResTimeStamp interfaces that weren't already limiting themselves to integer milliseconds.

The specific choice of how to coarsen the resolution is extremely bikesheddable; I commented the reasoning for my arbitrary choice but I admit it's arbitrary.

A couple tests of timing resolution still fail, but because they're seeing undefined and NaN values due to unimplemented attributes (#25658).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25656 fix #25296 fix #21276

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 10, 2020
Expose DOMHighResTimeStamps at lower res

As explained in https://www.w3.org/TR/hr-time-2/#clock-resolution and tested in a few WPT tests, we're not supposed to show Javascript the full resolution of OS timestamps. This fixes that on all the DOMHighResTimeStamp interfaces that weren't already limiting themselves to integer milliseconds.

The specific choice of how to coarsen the resolution is extremely bikesheddable; I commented the reasoning for my arbitrary choice but I admit it's arbitrary.

A couple tests of timing resolution still fail, but because they're seeing undefined and NaN values due to unimplemented attributes (#25658).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25656 fix #25296 fix #21276

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.