Skip to content
This repository has been archived by the owner on Nov 9, 2019. It is now read-only.

Try to somehow implement clock_res_get on Windows. #124

Merged
merged 6 commits into from
Nov 6, 2019

Conversation

marmistrz
Copy link
Member

@marmistrz marmistrz commented Oct 11, 2019

I don't like this implementation, and I'm not sure if we should merge it, but I think it's the best we can do without overly complicating stuff. We depend on the implementation details on std::time::{SystemTime, Instant} and cpu-time (on the underlying Windows syscalls).

While we could avoid it for ProcessTime and ThreadTime by adding an associated function returning the timer resolution, possibly the same for Instant (and wait forever until it's stable), but the problems for SystemTime are much more fundamental. We may have to reconsider this part of WASI API.

Depends on #117 and the implementation details of #119.

@sunfishcode
Copy link
Member

Unforunately, SystemTime is the most important one. My sense is that the approach in this PR is acceptable here, if that's our best option.

ProcessTime and ThreadTime may end up getting removed from WASI, since we generally don't want to require that WASI programs be one-to-one with host processes or host threads, and I don't know of an efficient way to implement them on popular OS's otherwise.

@marmistrz
Copy link
Member Author

I caught up with latest master.

@marmistrz
Copy link
Member Author

I'll probably need to open an issue in WebAssembly/WASI about these issues with implementation. They may make us reconsider the API spec.

@kubkon
Copy link
Member

kubkon commented Nov 3, 2019

I'll probably need to open an issue in WebAssembly/WASI about these issues with implementation. They may make us reconsider the API spec.

I'm not counting that out. However, before we do that, perhaps we should consult @peterhuene first? @peterhuene would you have any thoughts on this PR and the discussion in general?

@kubkon kubkon requested a review from peterhuene November 3, 2019 20:46
Copy link
Contributor

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

This looks great and you did some really in-depth investigation into how this could be well-implemented on Windows. Thanks for that!

Just some general feedback and a requested clarification on what the resolution value for the realtime clock is representing.

src/sys/windows/hostcalls_impl/misc.rs Outdated Show resolved Hide resolved
winx/src/time.rs Show resolved Hide resolved
src/sys/windows/hostcalls_impl/misc.rs Outdated Show resolved Hide resolved
@peterhuene peterhuene requested a review from kubkon November 5, 2019 21:12
Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

Great job, thanks!

@kubkon kubkon merged commit 2249405 into CraneStation:master Nov 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants