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
libs: Make bitarray functions lock parameters that aren't the caller #2081
Conversation
This now makes bit array functions which take in a second bit array, thread safe. Previously there was a warning on bitarray.Update to be lock the second parameter externally if thread safety wasrequired. This was not done within the codebase, so it was fine to change here. Closes #2080
Codecov Report
@@ Coverage Diff @@
## develop #2081 +/- ##
===========================================
- Coverage 62.75% 62.65% -0.11%
===========================================
Files 215 215
Lines 17277 17344 +67
===========================================
+ Hits 10843 10867 +24
- Misses 5546 5591 +45
+ Partials 888 886 -2
|
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.
👍 🍡
As commented inline would not do the bundled defer, for ease of editing and clarity of lifecycle.
defer func() { | ||
bA.mtx.Unlock() | ||
o.mtx.Unlock() | ||
}() |
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.
Why not:
a.mtx.Lock()
defer a.mtx.Unlock()
o.mtx.Lock
defer o.mtx.Unlock
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.
The reason I didn't bundle the defers is because my understanding is that each defer call is kinda slow: golang/go#14939
Do you think the performance should matter here? Its a bit of a micro-performance tweak, and its not at all clear that it applies here, so I'd be happy to switch to the more easy to maintain version :)
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.
As it is to be expected that BitArray
might be used in tight loops this is a valid concern.
defer func() { | ||
bA.mtx.Unlock() | ||
o.mtx.Unlock() | ||
}() |
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.
Same as above
This now makes bit array functions which take in a second bit array, thread
safe. Previously there was a warning on bitarray.Update to be lock the
second parameter externally if thread safety wasrequired.
This was not done within the codebase, so it was fine to change here.
Closes #2080