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

Increase precision of clocks in clock_time_get on Windows #182

Merged

Conversation

ospencer
Copy link
Member

This PR is rebased on #181.

This increases the precision of the UVWASI_CLOCK_PROCESS_CPUTIME_ID and UVWASI_CLOCK_THREAD_CPUTIME_ID clocks on Windows. The raw values from GetProcessTimes and GetThreadTimes are in units of 100 nanoseconds, which matches the reported resolution returned from uvwasi_clock_time_get.

I can't find any documentation on it, but in my testing I only saw an actual precision of about 1000 nanoseconds, which I suppose makes sense if the process/thread times are counted as microseconds.

@cjihrig
Copy link
Collaborator

cjihrig commented Jan 16, 2023

This has a conflict now.

@ospencer ospencer force-pushed the oscar/increase-windows-clock-times-precision branch from 55f4eb6 to 03c6435 Compare January 16, 2023 16:34
@ospencer
Copy link
Member Author

@cjihrig This should be all set, thanks.

@phated
Copy link
Contributor

phated commented Jan 16, 2023

@cjihrig Will you be able to cut a release with both these fixes and get them upstreamed into node 19? We're planning to recommend node 19+ WASI support in Grain.

@sunfishcode
Copy link

As a heads up here, the process and thread clocks are currently planned to be removed from future versions of WASI, because on implementations where there is not a one-to-one relationship between a wasm instance and a host process, it's prohibitively expensive to implement them. If you have use cases which depend on these clocks, please consider filing an issue in the wasi-clocks repo, where we can discuss how we might accomodate such use cases in a way that all WASI implementations can support.

@phated
Copy link
Contributor

phated commented Jan 16, 2023

@sunfishcode I don't think we have any direct need for them, but Grain is a WASI-compatible language so we have bindings for all of preview1 and discovered this bug. Updating uvwasi to preview2 is outlined in #59

@phated
Copy link
Contributor

phated commented Jan 16, 2023

(It also looks like someone needs to kick the CI because it's been running for 3 hours)

@cjihrig
Copy link
Collaborator

cjihrig commented Jan 16, 2023

Regarding the CI - I kicked the CI. It looks like GitHub Actions had some issue earlier on Windows. It looks like everything is fine now.

Regarding the release - yes, I can cut a new release and update Node. It will be released in Node 19 (either the next version or the one after, I'm not sure what the release calendar currently looks like).

Regarding the next WASI snapshot - someone needs to step up and do the work. There is a WIP branch here, but it hasn't been updated since Sept 2020. I'm happy to review code, cut releases, etc. but I no longer work on this project. I'm also happy to give the commit bit to anyone working on the project.

@cjihrig cjihrig merged commit 5d2ad07 into nodejs:main Jan 16, 2023
@phated
Copy link
Contributor

phated commented Jan 16, 2023

Thanks! We didn't know about that. This project is widely used (nodejs, WAMR, Wasm3) so we thought it was actively maintained. I'll try to figure out how we can keep this maintained! Thank you for all your hard work 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants