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

Make WASM polyfilled Instant work in web workers. #75

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stephanemagnenat
Copy link
Contributor

@stephanemagnenat stephanemagnenat commented Jul 20, 2023

The polyfill for std::Instant I added for WASM depends on window, but that later is not available in web workers. This PR uses the worker global scope, if present, and attempts to use window otherwise. If nothing works, it returns 0 instead of panicking.

@stephanemagnenat stephanemagnenat marked this pull request as draft July 20, 2023 11:05
@stephanemagnenat stephanemagnenat force-pushed the akaze/allow_performance_in_workers branch 5 times, most recently from 6c8ac68 to 44d5aed Compare July 20, 2023 11:22
@stephanemagnenat
Copy link
Contributor Author

This is blocked on rustwasm/wasm-bindgen#3530.

@stephanemagnenat stephanemagnenat force-pushed the akaze/allow_performance_in_workers branch from 8b68e54 to 8bb15fe Compare July 20, 2023 13:12
@daxpedda
Copy link

daxpedda commented Jul 20, 2023

The Instant you created here isn't comparable across threads (not sure if you even need that). You would have to take Performance.timeOrigin into account for that.

Shameless plug: I recommend you to use web-time, which does what you are doing here for you.

@stephanemagnenat
Copy link
Contributor Author

@daxpedda thanks for the review. Yes so we I did not consider cross-thread compatibility, as the thread story on the web is shaky anyway, and we do not have use cases for WASI at the moment. Also, I aimed for very simple implementation, as simplicity has value as well.

But I'm not against using a third-party crate that does the job more thoroughly. Let's see what the others think. @vadixidav, @xd009642 what do you think? Context is: support timing under WASM for Akaze performance instrumentation. Will have no effect on non-WASM targets.

@stephanemagnenat
Copy link
Contributor Author

@daxpedda a question: how does web-time compare to instant? Was there a specific reason for you not to the that one and create a new one?

@daxpedda
Copy link

web-time has a couple of advantages over instant:

@stephanemagnenat
Copy link
Contributor Author

stephanemagnenat commented Jul 21, 2023

Thanks for the explanation! Let's see what the others think about adding this dependency.

@xd009642
Copy link
Member

I don't have an objection to it given it's just for the wasm target

@cybersoulK
Copy link

i just had the following error in instant about 10% of the time in a web worker.

panicked at 'failed to get performance from global object: JsValue(undefined)
instant-0.1.12/src/wasm.rs:137:14

this crate works perfectly as a replacement. great 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.

4 participants