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

Remove some unsafes by using atomics #2186

Merged
merged 1 commit into from Dec 31, 2021

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented Nov 23, 2021

Description

Removes some uses of unsafe operations by substituting them with atomic operations. These are, due to the single-threaded nature of the current wasm target, lowered to normal operations on simple datatypes, as far as I know.

Uses Ordering::Relaxed since none of the operations are used as synchronization primitives.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code

voidpumpkin
voidpumpkin previously approved these changes Nov 23, 2021
@hamza1311
Copy link
Member

Web Assembly is going to support threads and atomics (see proposal). I'm not sure if this is a good solution to avoiding unsafe as atomics do come with a cost. I think safety comments are fine with unsafe blocks. If we really want to avoid unsafe, thread_locals would be a better solution

@WorldSEnder
Copy link
Member Author

WorldSEnder commented Nov 23, 2021

A shame that the proposal includes only sequential consistency instructions, which makes Ordering::Relaxed pointless. With a good implementation, that should have almost no cost for a boolean that is rarely modified. I'd be down for revisiting this when the proposal moves ahead?

thread_locals are hard to set globally in set_event_bubbling.

@hamza1311
Copy link
Member

I don't have a strict opinion on it. If it doesn't degrade the performance, I'm fine with keeping the atomics

@voidpumpkin
Copy link
Member

Blocked due to benchmarks not working atm

@voidpumpkin
Copy link
Member

@WorldSEnder could you rebase? Benchmarks are fixed but it will end up comparing older yew with master

@WorldSEnder
Copy link
Member Author

@voidpumpkin Done. The Benchmark check has successfully built, where can I see the results though? 😄

@hamza1311
Copy link
Member

@WorldSEnder benchmark results can be found here: https://github.com/yewstack/yew/runs/4659120701?check_suite_focus=true#step:15:24

I agree that we should make them more visible. I had to dig through the logs to find them

Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

04_select1k seems to be scuffed and shows performance decreases even when there are no changes.
03_update10th1k_x16 seems like it was a performance increase.
everything else stayed marginal
Though idk how much we can trust those benchmarks. For this PR it is enough though.

@siku2 siku2 merged commit c46cda1 into yewstack:master Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants