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

fix: use global() to get performance API on wasm32-unknown-unknown #107

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

Officeyutong
Copy link
Contributor

Currently quanta uses window.performance to access Performance API in browser, which will not work in WebWorkers. In WebWorkers, there is no window object, but performance is in the global space.

This PR uses web_sys::js_sys::global() to access the global scope, and look up for performance in that, which will both work on webpage and WebWorkers.

See https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Functions_and_classes_available_to_workers for more details

@Officeyutong Officeyutong marked this pull request as ready for review November 23, 2024 14:02
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR!

Taking a look at the code, it seems like this is going to be somewhat inefficient, reflecting through the global scope on every single call.

Is my intuition wrong here? Could we not instead simply query for the Performance class when constructing Monotonic so that there's always a live reference to it to directly use?

@Officeyutong
Copy link
Contributor Author

Thanks for opening this PR!

Taking a look at the code, it seems like this is going to be somewhat inefficient, reflecting through the global scope on every single call.

Is my intuition wrong here? Could we not instead simply query for the Performance class when constructing Monotonic so that there's always a live reference to it to directly use?

You are right, Reflection is relatively slow since its equal to object[attribute] in JS. I'll try to store a reference to Performance class to avoid frequent call to Reflection

@Officeyutong
Copy link
Contributor Author

Thanks for opening this PR!

Taking a look at the code, it seems like this is going to be somewhat inefficient, reflecting through the global scope on every single call.

Is my intuition wrong here? Could we not instead simply query for the Performance class when constructing Monotonic so that there's always a live reference to it to directly use?

Updated now. I also splitted wasm.rs to wasm_browser.rs and wasm_wasi.rs, since implementation of these two platforms are completely different

@Officeyutong
Copy link
Contributor Author

Hi @tobz , how's this issue going on?

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Apologies: life and work have been pretty busy lately. :)

I left a common about how we caching the Performance object, and in particular about how we're overriding the fact that it's !Send/!Sync.

src/clocks/monotonic/wasm_browser.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Overall, these changes seem reasonable to me. 👍🏻

@Officeyutong
Copy link
Contributor Author

Overall, these changes seem reasonable to me. 👍🏻

Hi, when can we get this PR merged

@tobz
Copy link
Member

tobz commented Dec 12, 2024

👋🏻

Going to merge these now, but I'll have to fix the MSRV and CI afterwards and make sure things are still green before I can cut a release.

@tobz tobz merged commit d1e11f0 into metrics-rs:main Dec 12, 2024
24 of 30 checks passed
@tobz
Copy link
Member

tobz commented Dec 13, 2024

Released as [email protected].

Thanks again for your contribution!

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.

2 participants