-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
More "thread-safe" LazyString #44936
Conversation
base/strings/lazy.jl
Outdated
end | ||
end | ||
return l.str | ||
old = swapfield!(l, :str, str, :acquire_release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You appear to have the @atomic
macros, so does this work?
old = swapfield!(l, :str, str, :acquire_release) | |
old = @atomicswap :acquire_release l.str = str |
But it is not actually the function you seem to have meant to call, which seems to be:
old = swapfield!(l, :str, str, :acquire_release) | |
old, = @atomicreplace :acquire_release l.str nothing => str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, thanks, I needed a CAS there.
We can't use the macros for operations due to the missing length(::Array)
method at this point. I juggled around the definitions just enough for @atomic field::T
. Or is it better to define some array methods earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it better to define some array methods earlier?
Maybe this is simple enough: b8dd080
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a serialize
method also, which drops the data and just keeps the string:
serialize(s::AbstractSerializer, l::LazyStr) =
serialize(s, _LazyStr((), string(l)))
Co-authored-by: Jameson Nash <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks!!
This patch makes
LazyString
thread-safe in the sense thatjulia/base/strings/lazy.jl
Lines 52 to 58 in 4b7b1fd
It's probably a non-issue ATM but it's also very cheap to do this anyway. I just noticed it while writing #44935.