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

The *_time macros are broken on wasm target, maybe disable them or give them a custom implementation? #3

Closed
StyMaar opened this issue Mar 31, 2022 · 3 comments

Comments

@StyMaar
Copy link

StyMaar commented Mar 31, 2022

Today, if you're using (even transitively) this crate on wasm, it will work fine, until at one point at runtime it panics at the point where one of the dependency uses debug_time! or any of the other *_time! macros, because Instant::now doesn't work in wasm.

panicked at 'time not implemented on this platform', library/std/src/sys/wasm/../unsupported/time.rs:13:9

Because it's a runtime crash, it can be difficult to reproduce and given how hard it is to debug a panic in WASM, it leads to a super frustrating experience.

Disabling these macros in wasm would at least make it a compile error, making it much easier to pinpoint where the problem comes from.

However it would be breaking change, since code that compiles fine today (and maybe crashes at runtime, or maybe not if you're lucky enough not to trigger the right condition) would stop compiling.

The alternative would involve writing a different implementation of MeasureTime for wasm: either by making them do exactly what the “non-time” macros do, or implementing the time measurement in a way that works for WASM (I'm not sure how possible it is in a general way, doing so for the browser is easy but what about non-browser wasm runtimes?)

@PSeitz
Copy link
Owner

PSeitz commented Mar 31, 2022

Thanks for the report.

I switched in this commit to the instant crate, which is wasm compatible. cd1b207
The config I chose would fallback to performance.now(), which should be available on most runtimes.

I think I can publish this as a bugfix release 0.8.1.

@StyMaar
Copy link
Author

StyMaar commented Mar 31, 2022

Nice, thanks.

@PSeitz
Copy link
Owner

PSeitz commented Apr 1, 2022

Published on cargo as 0.8.1

@PSeitz PSeitz closed this as completed Apr 1, 2022
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

No branches or pull requests

2 participants