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

Use vendored performance API to handle timing on WASM #3318

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

9SMTM6
Copy link
Contributor

@9SMTM6 9SMTM6 commented Sep 6, 2024

Before that this used wasm_timer, but only for Instant::now. But that panicked on web_workers, because it got there by going over the window, which isn't present in web workers.

This still doesn't work on all browsers though because of browser bugs, thus I added an assertion against usage on web-workers, that can be disabled if one so chooses.

This still needs documentation updates and a bit more testing to be done, but before that I wanted to discuss the implementation. Thus a draft PR.

See #3313.

Before that this used wasm_timer, but only for Instant::now. But that paniced on web_workers, because it got there by going over the window, which isnt present in web workers.

This still doesn't work on all browsers though, thus I added an assertion against usage on web-workers.
@9SMTM6 9SMTM6 force-pushed the dont_use_wasm_timer branch from 53eeaa3 to d4e8724 Compare September 7, 2024 06:16
@9SMTM6 9SMTM6 force-pushed the dont_use_wasm_timer branch from d4e8724 to c9974b4 Compare September 7, 2024 06:18
@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Sep 7, 2024

Note that this code, other than I expected (or at least hoped) still doesnt work on Windows, where performance.now isn't broken, which means theres still other things not working as expected.

@Dirbaio Note how I had to wrap the check for whether we're in a webworker into thread_local. I am wondering why the general static in time_driver_impl isn't in thread local storage. That's the one thing I can think of as potential issue right now. But then, would that not potentially be an issue elsewhere? And changing this, without branching the macro, would influence change other implementations.

Note I did of course try to comment out the executor on the main thread, to see if then the executor on the webworker would work fine, and it was still stuck.

If you don't think this is the issue, I don't know what else to try. As it stands this PR only fixes a panic where the remainder doesn't work anyways, and potentially (but not certainly) makes a complete fix easier for someone else, so I don't know if merging this is worth it.

If you think its worth it, I will add documentation of this particular issue and that there seem to be other issues, otherwise feel free to close this.

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.

1 participant