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.

Sign up for GitHub

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

Jump to bottom

Use arc_swap to implement AtomicStr #72

Merged
merged 6 commits into from
Jan 22, 2024
Merged

Use arc_swap to implement AtomicStr #72

merged 6 commits into from
Jan 22, 2024

Conversation

Kijewski
Copy link
Contributor

@Kijewski Kijewski commented Jan 19, 2024 •
edited
Loading

The previous implementation allowed use-after-free in a multi-threaded context. This PR fixes the problem by using ArcSwap, which is implements thread-safe swapping of Arcs, and is widely used.

Resolves #71.

@Kijewski
Use arc_swap to implement AtomicStr
b740582 
The previous implementation allowed use-after-free in a multi-threaded
context. This PR fixes the problem by using `ArcSwap`, which is
implements thread-safe swapping of `Arc`s, and is widely used.
@Kijewski
Copy link
Contributor Author

Kijewski commented Jan 19, 2024 •
edited
Loading 

t                  [+31.689% +32.305% +32.878%]
t_with_locale      [+4.2409% +5.2036% +5.9512%]
t_with_threads     [+4.1089% +4.5351% +4.9449%]
t_lorem_ipsum      [+35.164% +35.690% +36.188%]
t_with_args        [+5.4206% +5.8389% +6.2721%]
t_with_args (str)  [+4.6291% +5.1848% +5.7272%]
t_with_args (many) [+3.0211% +3.3934% +3.7793%]

Compared to fe72199.

@Kijewski
Use triomphe::Arc to mitigate performance losses
eba3abb 
This change replaces `std::sync::Arc` with `triomphe::Arc`. The latter
has no weak references, and is a lot faster because of that.
@Kijewski
Copy link
Contributor Author

Using triomph::Arc instead of std::sync::Arc seems to fully mitigate the performance loss that the first commit introduced:

t                  [+1.1192% +2.3614% +3.4182%]
t_with_locale      [-9.3043% -8.0077% -6.8903%]
t_with_threads     [-80.715% -80.342% -79.937%]
t_lorem_ipsum      [+1.8636% +2.7428% +3.6004%]
t_with_args        [+3.0018% +3.2772% +3.6441%]
t_with_args (str)  [+1.5075% +2.0956% +2.6433%]
t_with_args (many) [-6.1853% -5.9599% -5.6775%]

Compared to fe72199.

This would technically make this PR a breaking change, though, because the return type of rust_i18n::locale() was changed.

huacnlee
huacnlee requested changes Jan 22, 2024
View reviewed changes
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@huacnlee huacnlee merged commit 22e0609 into longbridgeapp:main Jan 22, 2024
@Kijewski Kijewski deleted the pr-arc-swap branch January 22, 2024 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huacnlee huacnlee huacnlee requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AtomicStr is unsound, causes use-after-free
2 participants
@Kijewski @huacnlee