Make WASM polyfilled Instant work in web workers.

rust-cv / cv

Rust CV mono-repo. Contains pure-Rust dependencies which attempt to encapsulate the capability of OpenCV, OpenMVG, and vSLAM frameworks in a cohesive set of APIs.

831 stars 64 forks source link

Make WASM polyfilled Instant work in web workers. #75

Open stephanemagnenat opened 1 year ago

stephanemagnenat commented 1 year ago

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 commented 1 year ago

This is blocked on https://github.com/rustwasm/wasm-bindgen/issues/3530.

daxpedda commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

web-time has a couple of advantages over instant:

Supports target_feature = "atomics".
Doesn't use Reflect to get the Performance object, which incurs a "big" performance penalty.
Caches the Performance object, to reduce any JS calls to an absolute minimum. See https://github.com/sebcrozet/instant/issues/21.
Be 100% API compatible with std::time. E.g. use SystemTimeError in SystemTime::duration_since().
Behaves exactly the same as std::time. E.g. Instant::duration_since() doesn't panic.
Doesn't have bugs, afaik. See https://github.com/sebcrozet/instant/issues/49.
Is maintained! (by me)

stephanemagnenat commented 1 year ago

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

xd009642 commented 1 year ago

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

cybersoulK commented 1 year ago

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!