Improve the support of atomics #5594
Conversation
lhchavez
force-pushed
the
git-atomics
branch
5 times, most recently
from
d60efac to
9bb17d5
Compare
|
I've only had a cursory 👀 so far, but I like what you're doing here. I want to look a bit more deeply, and I'd love @pks-t 's eyes as well. |
src/pool.c
Outdated
| if (git__page_size(&page_size) < 0) | ||
| page_size = 4096; | ||
| /* allow space for malloc overhead */ | ||
| size = (page_size - (2 * sizeof(void *)) - sizeof(git_pool_page)); | ||
| page_size -= (2 * sizeof(void *)) + sizeof(git_pool_page); | ||
| git_atomic_ssize_set(&cached_size, (int64_t)page_size); | ||
| } |
There was a problem hiding this comment.
As I said in the other MR already: I think we should just use a global initializer instead of penalizing each pool page allocation with atomics, which may be a heavyweight operation on some platforms.
There was a problem hiding this comment.
done, and even went a step further: since this is only used in the debug alloc pool, only initializing the system page size in that case. "don't pay for what you don't use" and stuff.
| # define GIT_BUILTIN_ATOMIC | ||
| # else | ||
| # define GIT_BUILTIN_SYNC | ||
| # endif |
There was a problem hiding this comment.
Do we provide any guarantees about our minimum required compiler versions?
There was a problem hiding this comment.
i'm kind of trusting the release docs in this case: https://www.gnu.org/software/gcc/gcc-4.7/changes.html
Support for atomic operations specifying the C++11/C11 memory model has been added. These new
__atomicroutines replace the existing__syncbuilt-in routines.
src/thread-utils.h
Outdated
| # endif | ||
|
|
||
| #elif defined(__GNUC__) | ||
|
|
||
| # if (__GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 1)) | ||
| # error Atomic primitives do not exist on this version of gcc; configure libgit2 with -DTHREADSAFE=OFF | ||
| # endif |
|
Thanks for doing this, @lchavez! The only thing I'm on the edge about is the pool page size. I think we should just move it into a global initializer and be done about it to not penalize pool usage with atomics. Other than that, things look good to me 👍 |
This change:
* Starts using GCC's and clang's `__atomic_*` intrinsics instead of the
`__sync_*` ones, since the former supercede the latter (and can be
safely replaced by their equivalent `__atomic_*` version with the
sequentially consistent model).
* Makes `git_atomic64`'s value `volatile`. Otherwise, this will make
ThreadSanitizer complain.
* Adds ways to load the values from atomics. As it turns out,
unsynchronized read are okay only in some architectures, but if we
want to be correct (and make ThreadSanitizer happy), those loads
should also be performed with the atomic builtins.
* Fixes two ThreadSanitizer warnings, as a proof-of-concept that this
works:
- Avoid directly accessing `git_refcount`'s `owner` directly, and
instead makes all callers go through the `GIT_REFCOUNT_*()` macros,
which also use the atomic utilities.
- Makes `pool_system_page_size()` race-free.
Part of: libgit2#5592
Instead, globally initialize the system page size.
Done and rebased while I was at it. |
|
Thanks @lhchavez! |
This change:
__atomic_*intrinsics instead of the__sync_*ones, since the former supercede the latter (and can besafely replaced by their equivalent
__atomic_*version with thesequentially consistent model).
git_atomic64's valuevolatile. Otherwise, this will makeThreadSanitizer complain.
unsynchronized read are okay only in some architectures, but if we
want to be correct (and make ThreadSanitizer happy), those loads
should also be performed with the atomic builtins.
works:
git_refcount'sownerdirectly, andinstead makes all callers go through the
GIT_REFCOUNT_*()macros,which also use the atomic utilities.
pool_system_page_size()race-free.Part of: #5592