temporal: support nanosecond precision from platform [v8/v8 : main]
snek (Gerrit)
snek voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Leszek Swirski
- Manish Goregaokar
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
snek (Gerrit)
snek added 1 comment![]( "Open in Gerrit")
V8::GetCurrentPlatform()->CurrentClockTimeMillisecondsHighResolution();can we just change this API to return a better value? this is the only consumer of this api and the host is likely getting it from something shaped like a timespec and then converting it into a double, and then this code is converting it back...
Related details
- Leszek Swirski
- Manish Goregaokar
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar added 1 comment![]( "Open in Gerrit")
I'd rather not maintain more I128Nanoseconds-creating code if possible
Could this instead be written as `std::unique_ptr<temporal_rs::Instant> SystemUTCInstant()`? This way our impl can continue to call SystemUTCEpochMilliseconds, and you can patch it to have ns instead?
Related details
- Leszek Swirski
- snek
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
snek (Gerrit)
snek added 1 comment![]( "Open in Gerrit")
I'd rather not maintain more I128Nanoseconds-creating code if possible
Could this instead be written as `std::unique_ptr<temporal_rs::Instant> SystemUTCInstant()`? This way our impl can continue to call SystemUTCEpochMilliseconds, and you can patch it to have ns instead?
what? is there some history here i'm not aware of? it seems kind of bonkers to say that node and deno should have to patch v8 /forever/ to make this work. especially since there is already a platform api to provide high resolution time?
Related details
- Leszek Swirski
- Manish Goregaokar
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar added 1 comment![]( "Open in Gerrit")
snekI'd rather not maintain more I128Nanoseconds-creating code if possible
Could this instead be written as `std::unique_ptr<temporal_rs::Instant> SystemUTCInstant()`? This way our impl can continue to call SystemUTCEpochMilliseconds, and you can patch it to have ns instead?
what? is there some history here i'm not aware of? it seems kind of bonkers to say that node and deno should have to patch v8 /forever/ to make this work. especially since there is already a platform api to provide high resolution time?
I'm confused: I was under the impression that this *was* a forever-patch, since the current code is still using the milliseconds API, no?
I guess the difference is that in node it is able to return a real double rather than an integer stored as a double?
Related details
- Leszek Swirski
- snek
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Manish Goregaokar
| Code-Review | +1 |
3 comments
snekI'd rather not maintain more I128Nanoseconds-creating code if possible
Could this instead be written as `std::unique_ptr<temporal_rs::Instant> SystemUTCInstant()`? This way our impl can continue to call SystemUTCEpochMilliseconds, and you can patch it to have ns instead?
Manish Goregaokarwhat? is there some history here i'm not aware of? it seems kind of bonkers to say that node and deno should have to patch v8 /forever/ to make this work. especially since there is already a platform api to provide high resolution time?
I'm confused: I was under the impression that this *was* a forever-patch, since the current code is still using the milliseconds API, no?
I guess the difference is that in node it is able to return a real double rather than an integer stored as a double?
Ah, I see, yeah. Node returns a real double and Chromium doesn't, and that's the primary difference, so that won't need a forever patch. I was thrown off by "milliseconds"
This seems fine to me especially since it's using absl's i128 API.
V8::GetCurrentPlatform()->CurrentClockTimeMillisecondsHighResolution();can we just change this API to return a better value? this is the only consumer of this api and the host is likely getting it from something shaped like a timespec and then converting it into a double, and then this code is converting it back...
that would be really nice if possible. I can't make a call on that, though, Leszek may be able to.
Related details
- Leszek Swirski
- snek
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
snek (Gerrit)
snek added 1 comment![]( "Open in Gerrit")
snekI'd rather not maintain more I128Nanoseconds-creating code if possible
Could this instead be written as `std::unique_ptr<temporal_rs::Instant> SystemUTCInstant()`? This way our impl can continue to call SystemUTCEpochMilliseconds, and you can patch it to have ns instead?
Manish Goregaokarwhat? is there some history here i'm not aware of? it seems kind of bonkers to say that node and deno should have to patch v8 /forever/ to make this work. especially since there is already a platform api to provide high resolution time?
I'm confused: I was under the impression that this *was* a forever-patch, since the current code is still using the milliseconds API, no?
I guess the difference is that in node it is able to return a real double rather than an integer stored as a double?
so basically someone reported this: https://github.com/denoland/deno/issues/31790 and i'm trying to resolve that issue. the final hurdle i hit is that the code here is working in milliseconds even though the host can otherwise provide higher resolution. based on the comment that i removed in this CL, my assumption was that this was just done for convenience, because that missing precision does not matter in chrome.
i think from an embedder perspective, i'd expect v8 to make full use of whatever resolution the "high resolution" api can provide. so in chrome, they can truncate it to milliseconds, and in deno/node we can let it use whatever resolution we get from the os.
so with that context, i'm curious what you'd suggest here. i'd really prefer not to maintain patches on top of v8 to get this working.
Related details
- Leszek Swirski
- Manish Goregaokar
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar added 1 comment![]( "Open in Gerrit")
snekI'd rather not maintain more I128Nanoseconds-creating code if possible
Could this instead be written as `std::unique_ptr<temporal_rs::Instant> SystemUTCInstant()`? This way our impl can continue to call SystemUTCEpochMilliseconds, and you can patch it to have ns instead?
Manish Goregaokarwhat? is there some history here i'm not aware of? it seems kind of bonkers to say that node and deno should have to patch v8 /forever/ to make this work. especially since there is already a platform api to provide high resolution time?
snekI'm confused: I was under the impression that this *was* a forever-patch, since the current code is still using the milliseconds API, no?
I guess the difference is that in node it is able to return a real double rather than an integer stored as a double?
so basically someone reported this: https://github.com/denoland/deno/issues/31790 and i'm trying to resolve that issue. the final hurdle i hit is that the code here is working in milliseconds even though the host can otherwise provide higher resolution. based on the comment that i removed in this CL, my assumption was that this was just done for convenience, because that missing precision does not matter in chrome.
i think from an embedder perspective, i'd expect v8 to make full use of whatever resolution the "high resolution" api can provide. so in chrome, they can truncate it to milliseconds, and in deno/node we can let it use whatever resolution we get from the os.
so with that context, i'm curious what you'd suggest here. i'd really prefer not to maintain patches on top of v8 to get this working.
Yeah, like I said, I misread the code and thought that CurrentClockTimeMillisecondsHighResolution only returned an integral milliseconds value, and that deno intended to patch this code further to use some kind of nanosecond API instead.
It returns a double, so it can choose to return higher precision values without needing a patch here..
So this current CL is fine.
Related details
- Leszek Swirski
- snek
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
V8::GetCurrentPlatform()->CurrentClockTimeMillisecondsHighResolution();Manish Goregaokarcan we just change this API to return a better value? this is the only consumer of this api and the host is likely getting it from something shaped like a timespec and then converting it into a double, and then this code is converting it back...
that would be really nice if possible. I can't make a call on that, though, Leszek may be able to.
the platform stuff is public API so we can't change it trivially, but we could go through the whole API deprecation process. It's also provided by the embedder, so you'd have to change the return value provided by Chrome, which overloads these virtual methods to do timer jittering for mitigating timing attacks (https://source.chromium.org/chromium/chromium/src/+/main:gin/v8_platform.cc;l=339;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3). Not difficult to do as such, I think, just a bit tedious.
Related details
- snek
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
snek (Gerrit)
snek voted and added 1 comment![]( "Open in Gerrit")
Votes added by snek
| Commit-Queue | +2 |
1 comment
V8::GetCurrentPlatform()->CurrentClockTimeMillisecondsHighResolution();Manish Goregaokarcan we just change this API to return a better value? this is the only consumer of this api and the host is likely getting it from something shaped like a timespec and then converting it into a double, and then this code is converting it back...
Leszek Swirskithat would be really nice if possible. I can't make a call on that, though, Leszek may be able to.
the platform stuff is public API so we can't change it trivially, but we could go through the whole API deprecation process. It's also provided by the embedder, so you'd have to change the return value provided by Chrome, which overloads these virtual methods to do timer jittering for mitigating timing attacks (https://source.chromium.org/chromium/chromium/src/+/main:gin/v8_platform.cc;l=339;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3). Not difficult to do as such, I think, just a bit tedious.
ok, I'll consider this for a follow-up then.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
temporal: support nanosecond precision from platform
non-web embedders don't need to artificially restrict the precision of
timestamps, so support nanoseconds if the platform is willing to provide
them.
- M src/objects/js-temporal-objects.cc
Code-Review: +1 by Leszek Swirski, +1 by Manish Goregaokar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |