From 5dc55bea5d6ce18eabd93567b5fb24be60aa8cac Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Mon, 8 May 2023 13:38:40 -0400 Subject: [PATCH 01/13] Using atomic counters on shared containers --- include/roaring/containers/containers.h | 15 ++++++++++++- include/roaring/portability.h | 19 ++++++++++++++++ src/containers/containers.c | 29 ++++++++++++++++++++++--- src/roaring.c | 11 ++++++++++ 4 files changed, 70 insertions(+), 4 deletions(-) diff --git a/include/roaring/containers/containers.h b/include/roaring/containers/containers.h index b1391b529..b25d10866 100644 --- a/include/roaring/containers/containers.h +++ b/include/roaring/containers/containers.h @@ -55,12 +55,25 @@ extern "C" { namespace roaring { namespace internal { * A shared container is a wrapper around a container * with reference counting. */ - +#if CROARING_C_ATOMIC +STRUCT_CONTAINER(shared_container_s) { + container_t *container; + uint8_t typecode; + _Atomic(uint32_t) counter; // to be managed atomically +}; +#elif CROARING_CPP_ATOMIC +STRUCT_CONTAINER(shared_container_s) { + container_t *container; + uint8_t typecode; + std::atomic counter; // to be managed atomically +}; +#else STRUCT_CONTAINER(shared_container_s) { container_t *container; uint8_t typecode; uint32_t counter; // to be managed atomically }; +#endif typedef struct shared_container_s shared_container_t; diff --git a/include/roaring/portability.h b/include/roaring/portability.h index f05206272..38cc72345 100644 --- a/include/roaring/portability.h +++ b/include/roaring/portability.h @@ -38,6 +38,9 @@ #define CROARING_REGULAR_VISUAL_STUDIO 1 #endif // __clang__ #endif // _MSC_VER +#ifndef CROARING_VISUAL_STUDIO +#define CROARING_VISUAL_STUDIO 0 +#endif #if defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE < 200809L) #undef _POSIX_C_SOURCE @@ -395,6 +398,22 @@ static inline int roaring_hamming(uint64_t x) { #endif // __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ #endif + +#if defined(__cplusplus) +#define CROARING_CPP_ATOMIC 1 +#define CROARING_C_ATOMIC 0 +#include +#elif CROARING_VISUAL_STUDIO +// in C under Visual Studio, we use the C++ atomics? +#define CROARING_ATOMIC 0 +#define CROARING_CPP_ATOMIC 0 +#include +#else // C +#define CROARING_C_ATOMIC 1 +#define CROARING_CPP_ATOMIC 0 +#include +#endif + // We need portability.h to be included first, // but we also always want isadetection.h to be // included (right after). diff --git a/src/containers/containers.c b/src/containers/containers.c index c2fd32942..1667a1448 100644 --- a/src/containers/containers.c +++ b/src/containers/containers.c @@ -137,7 +137,13 @@ container_t *get_copy_of_container( shared_container_t *shared_container; if (*typecode == SHARED_CONTAINER_TYPE) { shared_container = CAST_shared(c); +#if CROARING_C_ATOMIC + atomic_fetch_add(&(shared_container->counter), 1); +#elif CROARING_CPP_ATOMIC + std::atomic_fetch_add(&(shared_container->counter), 1); +#else shared_container->counter += 1; +#endif return shared_container; } assert(*typecode != SHARED_CONTAINER_TYPE); @@ -149,8 +155,13 @@ container_t *get_copy_of_container( shared_container->container = c; shared_container->typecode = *typecode; - +#if CROARING_C_ATOMIC + atomic_store(&(shared_container->counter), 2); +#elif CROARING_CPP_ATOMIC + std::atomic_store(&(shared_container->counter), 2); +#else shared_container->counter = 2; +#endif *typecode = SHARED_CONTAINER_TYPE; return shared_container; @@ -188,12 +199,18 @@ container_t *container_clone(const container_t *c, uint8_t typecode) { container_t *shared_container_extract_copy( shared_container_t *sc, uint8_t *typecode ){ - assert(sc->counter > 0); assert(sc->typecode != SHARED_CONTAINER_TYPE); - sc->counter--; *typecode = sc->typecode; container_t *answer; +#if CROARING_C_ATOMIC + if(atomic_fetch_sub(&(sc->counter), 1) == 1) { +#elif CROARING_CPP_ATOMIC + if(std::atomic_fetch_sub(&(sc->counter), 1) == 1) { +#else + assert(sc->counter > 0); + sc->counter--; if (sc->counter == 0) { +#endif answer = sc->container; sc->container = NULL; // paranoid roaring_free(sc); @@ -205,9 +222,15 @@ container_t *shared_container_extract_copy( } void shared_container_free(shared_container_t *container) { +#if CROARING_C_ATOMIC + if(atomic_fetch_sub(&(container->counter), 1) == 1) { +#elif CROARING_CPP_ATOMIC + if(std::atomic_fetch_sub(&(container->counter), 1) == 1) { +#else assert(container->counter > 0); container->counter--; if (container->counter == 0) { +#endif assert(container->typecode != SHARED_CONTAINER_TYPE); container_free(container->container, container->typecode); container->container = NULL; // paranoid diff --git a/src/roaring.c b/src/roaring.c index 234356e0f..631f2c7fe 100644 --- a/src/roaring.c +++ b/src/roaring.c @@ -348,9 +348,20 @@ void roaring_bitmap_printf_describe(const roaring_bitmap_t *r) { get_full_container_name(ra->containers[i], ra->typecodes[i]), container_get_cardinality(ra->containers[i], ra->typecodes[i])); if (ra->typecodes[i] == SHARED_CONTAINER_TYPE) { +#if CROARING_C_ATOMIC + printf( + "(shared count = %" PRIu32 " )", + atomic_load(&(CAST_shared(ra->containers[i])->counter))); +#elif CROARING_CPP_ATOMIC + printf( + "(shared count = %" PRIu32 " )", + std::atomic_load(&(CAST_shared(ra->containers[i])->counter))); +#else printf( "(shared count = %" PRIu32 " )", CAST_shared(ra->containers[i])->counter); +#endif + } if (i + 1 < ra->size) { From e7b3fb13ee953a27c92e1848f5023a8254727ec5 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Mon, 8 May 2023 13:58:46 -0400 Subject: [PATCH 02/13] Getting around VS limitations. --- include/roaring/portability.h | 11 ++++++++--- src/isadetection.c | 21 ++++++++++----------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/include/roaring/portability.h b/include/roaring/portability.h index 38cc72345..0fcea96db 100644 --- a/include/roaring/portability.h +++ b/include/roaring/portability.h @@ -41,6 +41,12 @@ #ifndef CROARING_VISUAL_STUDIO #define CROARING_VISUAL_STUDIO 0 #endif +#ifndef CROARING_CLANG_VISUAL_STUDIO +#define CROARING_CLANG_VISUAL_STUDIO 0 +#endif +#ifndef CROARING_REGULAR_VISUAL_STUDIO +#define CROARING_REGULAR_VISUAL_STUDIO 0 +#endif #if defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE < 200809L) #undef _POSIX_C_SOURCE @@ -403,11 +409,10 @@ static inline int roaring_hamming(uint64_t x) { #define CROARING_CPP_ATOMIC 1 #define CROARING_C_ATOMIC 0 #include -#elif CROARING_VISUAL_STUDIO -// in C under Visual Studio, we use the C++ atomics? +#elif defined(__STDC_NO_ATOMICS__) || CROARING_REGULAR_VISUAL_STUDIO +// https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/ #define CROARING_ATOMIC 0 #define CROARING_CPP_ATOMIC 0 -#include #else // C #define CROARING_C_ATOMIC 1 #define CROARING_CPP_ATOMIC 0 diff --git a/src/isadetection.c b/src/isadetection.c index 06866f4cf..c611a0e94 100644 --- a/src/isadetection.c +++ b/src/isadetection.c @@ -200,27 +200,26 @@ static inline uint32_t croaring_detect_supported_architectures() { static uint32_t buffer = dynamic_croaring_detect_supported_architectures(); return buffer; } -#elif CROARING_VISUAL_STUDIO -// Visual Studio does not support C11 atomics. -static inline uint32_t croaring_detect_supported_architectures() { - static int buffer = CROARING_UNINITIALIZED; +#elif CROARING_C_ATOMIC +uint32_t croaring_detect_supported_architectures() { + // we use an atomic for thread safety + static _Atomic uint32_t buffer = CROARING_UNINITIALIZED; if (buffer == CROARING_UNINITIALIZED) { + // atomicity is sufficient buffer = dynamic_croaring_detect_supported_architectures(); } return buffer; } -#else // CROARING_VISUAL_STUDIO -#include -uint32_t croaring_detect_supported_architectures() { - // we use an atomic for thread safety - static _Atomic uint32_t buffer = CROARING_UNINITIALIZED; +#else +// If we do not have C11 atomics, we do the best we can. +static inline uint32_t croaring_detect_supported_architectures() { + static int buffer = CROARING_UNINITIALIZED; if (buffer == CROARING_UNINITIALIZED) { - // atomicity is sufficient buffer = dynamic_croaring_detect_supported_architectures(); } return buffer; } -#endif // CROARING_REGULAR_VISUAL_STUDIO +#endif // CROARING_C_ATOMIC #ifdef ROARING_DISABLE_AVX From 50a1b926f445ff4f67c0cfa62959db3518d9a378 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Mon, 8 May 2023 14:06:52 -0400 Subject: [PATCH 03/13] Adding thread sanitizer CI --- .github/workflows/ubuntu-sani-thread-ci.yml | 27 +++++++++++++++++++++ CMakeLists.txt | 2 ++ tools/cmake/FindOptions.cmake | 4 ++- 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/ubuntu-sani-thread-ci.yml diff --git a/.github/workflows/ubuntu-sani-thread-ci.yml b/.github/workflows/ubuntu-sani-thread-ci.yml new file mode 100644 index 000000000..067644aea --- /dev/null +++ b/.github/workflows/ubuntu-sani-thread-ci.yml @@ -0,0 +1,27 @@ +name: Ubuntu-Sanitized-CI + +'on': + - push + - pull_request + +permissions: + contents: read + +jobs: + ci: + name: ubuntu-gcc + runs-on: ubuntu-latest + + env: + CC: gcc + CXX: g++ + + steps: + - uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v2.7.0 + - name: Build and Test + run: | + mkdir build + cd build + cmake -DROARING_SANITIZE_THREADS=ON .. + cmake --build . + ctest . --output-on-failure diff --git a/CMakeLists.txt b/CMakeLists.txt index 16f9cf396..caf451287 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -44,6 +44,8 @@ option(ROARING_BUILD_LTO "Build library with Link Time Optimization" OFF) option(ROARING_BUILD_C_AS_CPP "Build library C files using C++ compilation" OFF) option(ROARING_BUILD_C_TESTS_AS_CPP "Build test C files using C++ compilation" OFF) option(ROARING_SANITIZE "Sanitize addresses" OFF) +option(ROARING_THREADS "Sanitize threads" OFF) + option(ENABLE_ROARING_TESTS "If OFF, disable unit tests altogether" ON) set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/tools/cmake") diff --git a/tools/cmake/FindOptions.cmake b/tools/cmake/FindOptions.cmake index f7b8f9675..e1d1a036e 100644 --- a/tools/cmake/FindOptions.cmake +++ b/tools/cmake/FindOptions.cmake @@ -12,7 +12,9 @@ if(ROARING_SANITIZE) append(CMAKE_SHARED_LINKER_FLAGS "-fuse-ld=gold") endif() endif() - +if(ROARING_SANITIZE_THREADS) + set(ROARING_SANITIZE_FLAGS "-fsanitize=thread -fno-sanitize-recover=all") +endif() if((NOT MSVC) AND ROARING_ARCH) set(OPT_FLAGS "-march=${ROARING_ARCH}") endif() From b96ded531f46b3723cd336469d321e25f2e4113a Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Tue, 9 May 2023 16:22:21 -0400 Subject: [PATCH 04/13] Adding tests --- CMakeLists.txt | 2 +- include/roaring/portability.h | 12 ++++- tests/CMakeLists.txt | 20 +++++++++ tests/threads_unit.c | 84 +++++++++++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 tests/threads_unit.c diff --git a/CMakeLists.txt b/CMakeLists.txt index caf451287..e52be498c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -44,7 +44,7 @@ option(ROARING_BUILD_LTO "Build library with Link Time Optimization" OFF) option(ROARING_BUILD_C_AS_CPP "Build library C files using C++ compilation" OFF) option(ROARING_BUILD_C_TESTS_AS_CPP "Build test C files using C++ compilation" OFF) option(ROARING_SANITIZE "Sanitize addresses" OFF) -option(ROARING_THREADS "Sanitize threads" OFF) +option(ROARING_SANITIZE_THREADS "Sanitize threads" OFF) option(ENABLE_ROARING_TESTS "If OFF, disable unit tests altogether" ON) diff --git a/include/roaring/portability.h b/include/roaring/portability.h index 0fcea96db..69ff57400 100644 --- a/include/roaring/portability.h +++ b/include/roaring/portability.h @@ -405,10 +405,20 @@ static inline int roaring_hamming(uint64_t x) { #endif -#if defined(__cplusplus) +#if defined(__cplusplus) && __cplusplus >= 201103L +#ifdef __has_include +#if __has_include() +// It is now safe: #define CROARING_CPP_ATOMIC 1 #define CROARING_C_ATOMIC 0 #include +#endif //__has_include() +#else +// We lack __has_include to check: +#define CROARING_CPP_ATOMIC 1 +#define CROARING_C_ATOMIC 0 +#include +#endif //__has_include #elif defined(__STDC_NO_ATOMICS__) || CROARING_REGULAR_VISUAL_STUDIO // https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/ #define CROARING_ATOMIC 0 diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 28177c01a..0b4d70b3a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -20,6 +20,26 @@ add_c_test(format_portability_unit) add_c_test(robust_deserialization_unit) add_c_test(container_comparison_unit) add_c_test(add_offset) +find_package(Threads) +if(Threads_FOUND) + message(STATUS "Your system supports threads.") + IF(APPLE) + message(STATUS "Apple.") + set(CMAKE_THREAD_LIBS_INIT "-lpthread") + set(CMAKE_HAVE_THREADS_LIBRARY 1) + set(CMAKE_USE_WIN32_THREADS_INIT 0) + set(CMAKE_USE_PTHREADS_INIT 1) + set(THREADS_PREFER_PTHREAD_FLAG ON) + ENDIF() + if(ROARING_BUILD_C_TESTS_AS_CPP) # under C++, container_t* != void* + SET_SOURCE_FILES_PROPERTIES(threads_unit.c PROPERTIES LANGUAGE CXX) + endif() + add_executable(threads_unit threads_unit.c) + target_link_libraries(threads_unit PRIVATE roaring Threads::Threads) + add_test(threads_unit threads_unit) +else(Threads_FOUND) + message(STATUS "Your system does not support threads.") +endif(Threads_FOUND) if (NOT WIN32) # We exclude POSIX tests from Microsoft Windows diff --git a/tests/threads_unit.c b/tests/threads_unit.c new file mode 100644 index 000000000..65d9d4604 --- /dev/null +++ b/tests/threads_unit.c @@ -0,0 +1,84 @@ +#include +#include + +#if __STDC_NO_THREADS__ +int main() { + printf("This test requires threads support.\n"); + return EXIT_SUCCESS; +} +#else //__STDC_NO_THREADS__ +#include +#include +#include + +// helper function to create a roaring bitmap from an array +static roaring_bitmap_t *make_roaring_from_array(uint32_t *a, int len) { + roaring_bitmap_t *r1 = roaring_bitmap_create(); + for (int i = 0; i < len; ++i) roaring_bitmap_add(r1, a[i]); + return r1; +} + +// We are mostly running this test to check for data races suing thread sanitizer. +int run(void * input) { + roaring_bitmap_t **rarray = (roaring_bitmap_t **)input; + roaring_bitmap_t *r1 = roaring_bitmap_copy(rarray[0]); + roaring_bitmap_t *r2 = rarray[1]; + roaring_bitmap_t *r3 = rarray[2]; + roaring_bitmap_and_inplace(r1, r2); + roaring_bitmap_and_inplace(r1, r3); + int answer = (int)roaring_bitmap_get_cardinality(r1); + roaring_bitmap_free(r1); + return answer; +} + +bool run_threads_unit_tests() { + uint32_t *ans = (uint32_t*)calloc(100000, sizeof(int32_t)); + + for (uint32_t i = 0; i < 50000; i++) { + if (i != 300) { + ans[ans_ctr++] = 65536 + i; + } + } + for (uint32_t i = 50000; i < 150000; i++) { + if ((i%500) == 0) { + ans[ans_ctr++] = i; + } + } + for (uint32_t i = 150000; i < 200000; i++) { + if ((i%2) == 0) { + ans[ans_ctr++] = i; + } + } + + roaring_bitmap_t *r1 = make_roaring_from_array(ans, ans_ctr); + free(arr); + + roaring_bitmap_set_copy_on_write(r1, true); + roaring_bitmap_run_optimize(r1); + roaring_bitmap_t *r2 = roaring_bitmap_of(5, 10010,10020,10030,10040,10050); + roaring_bitmap_set_copy_on_write(r2, true); + roaring_bitmap_t *r3 = roaring_bitmap_copy(r1); + roaring_bitmap_t* rarray[] = {r1, r2, r3}; + + thrd_t thread1; + thrd_t thread2; + int result1{}; + int result2{}; + + thrd_create(&thread1, run, rarray); + thrd_create(&thread2, run, rarray); + thrd_join(&thread1, &result1); + thrd_join(&thread2, &result2); + roaring_bitmap_free(r1); + roaring_bitmap_free(r2); + roaring_bitmap_free(r3); + return result1 == result2; +} + +int main() { + tellmeall(); + bool is_ok = run_threads_unit_tests(); + return is_ok: EXIT_SUCCESS ? EXIT_FAILURE; +} + +#endif // __STDC_NO_THREADS__ \ No newline at end of file From fd8eb9f8da0b9169c642cdd1c55e0621f4c07ab1 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Tue, 9 May 2023 16:22:42 -0400 Subject: [PATCH 05/13] Removing unnecessary code --- tests/CMakeLists.txt | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0b4d70b3a..d823c5d7a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,14 +23,6 @@ add_c_test(add_offset) find_package(Threads) if(Threads_FOUND) message(STATUS "Your system supports threads.") - IF(APPLE) - message(STATUS "Apple.") - set(CMAKE_THREAD_LIBS_INIT "-lpthread") - set(CMAKE_HAVE_THREADS_LIBRARY 1) - set(CMAKE_USE_WIN32_THREADS_INIT 0) - set(CMAKE_USE_PTHREADS_INIT 1) - set(THREADS_PREFER_PTHREAD_FLAG ON) - ENDIF() if(ROARING_BUILD_C_TESTS_AS_CPP) # under C++, container_t* != void* SET_SOURCE_FILES_PROPERTIES(threads_unit.c PROPERTIES LANGUAGE CXX) endif() From e66e9c0086f373001417a7e6375db07dc0b3399b Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Tue, 9 May 2023 18:20:36 -0400 Subject: [PATCH 06/13] Adding tests for data races --- tests/CMakeLists.txt | 14 ++++-- tests/threads_unit.c | 84 ----------------------------------- tests/threads_unit.cpp | 64 ++++++++++++++++++++++++++ tools/cmake/FindOptions.cmake | 4 +- 4 files changed, 75 insertions(+), 91 deletions(-) delete mode 100644 tests/threads_unit.c create mode 100644 tests/threads_unit.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d823c5d7a..d7fd398ba 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,11 +23,17 @@ add_c_test(add_offset) find_package(Threads) if(Threads_FOUND) message(STATUS "Your system supports threads.") - if(ROARING_BUILD_C_TESTS_AS_CPP) # under C++, container_t* != void* - SET_SOURCE_FILES_PROPERTIES(threads_unit.c PROPERTIES LANGUAGE CXX) - endif() - add_executable(threads_unit threads_unit.c) + add_executable(threads_unit threads_unit.cpp) target_link_libraries(threads_unit PRIVATE roaring Threads::Threads) + if(ROARING_SANITIZE_THREADS) + # libtsan might be needed + if(CMAKE_SYSTEM_NAME STREQUAL "Linux") + message(STATUS "Under Linux, you may need to install libtsan." ) + endif() + target_compile_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) + target_link_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) + message(STATUS "Sanitizing threads.") + endif() add_test(threads_unit threads_unit) else(Threads_FOUND) message(STATUS "Your system does not support threads.") diff --git a/tests/threads_unit.c b/tests/threads_unit.c deleted file mode 100644 index 65d9d4604..000000000 --- a/tests/threads_unit.c +++ /dev/null @@ -1,84 +0,0 @@ -#include -#include - -#if __STDC_NO_THREADS__ -int main() { - printf("This test requires threads support.\n"); - return EXIT_SUCCESS; -} -#else //__STDC_NO_THREADS__ -#include -#include -#include - -// helper function to create a roaring bitmap from an array -static roaring_bitmap_t *make_roaring_from_array(uint32_t *a, int len) { - roaring_bitmap_t *r1 = roaring_bitmap_create(); - for (int i = 0; i < len; ++i) roaring_bitmap_add(r1, a[i]); - return r1; -} - -// We are mostly running this test to check for data races suing thread sanitizer. -int run(void * input) { - roaring_bitmap_t **rarray = (roaring_bitmap_t **)input; - roaring_bitmap_t *r1 = roaring_bitmap_copy(rarray[0]); - roaring_bitmap_t *r2 = rarray[1]; - roaring_bitmap_t *r3 = rarray[2]; - roaring_bitmap_and_inplace(r1, r2); - roaring_bitmap_and_inplace(r1, r3); - int answer = (int)roaring_bitmap_get_cardinality(r1); - roaring_bitmap_free(r1); - return answer; -} - -bool run_threads_unit_tests() { - uint32_t *ans = (uint32_t*)calloc(100000, sizeof(int32_t)); - - for (uint32_t i = 0; i < 50000; i++) { - if (i != 300) { - ans[ans_ctr++] = 65536 + i; - } - } - for (uint32_t i = 50000; i < 150000; i++) { - if ((i%500) == 0) { - ans[ans_ctr++] = i; - } - } - for (uint32_t i = 150000; i < 200000; i++) { - if ((i%2) == 0) { - ans[ans_ctr++] = i; - } - } - - roaring_bitmap_t *r1 = make_roaring_from_array(ans, ans_ctr); - free(arr); - - roaring_bitmap_set_copy_on_write(r1, true); - roaring_bitmap_run_optimize(r1); - roaring_bitmap_t *r2 = roaring_bitmap_of(5, 10010,10020,10030,10040,10050); - roaring_bitmap_set_copy_on_write(r2, true); - roaring_bitmap_t *r3 = roaring_bitmap_copy(r1); - roaring_bitmap_t* rarray[] = {r1, r2, r3}; - - thrd_t thread1; - thrd_t thread2; - int result1{}; - int result2{}; - - thrd_create(&thread1, run, rarray); - thrd_create(&thread2, run, rarray); - thrd_join(&thread1, &result1); - thrd_join(&thread2, &result2); - roaring_bitmap_free(r1); - roaring_bitmap_free(r2); - roaring_bitmap_free(r3); - return result1 == result2; -} - -int main() { - tellmeall(); - bool is_ok = run_threads_unit_tests(); - return is_ok: EXIT_SUCCESS ? EXIT_FAILURE; -} - -#endif // __STDC_NO_THREADS__ \ No newline at end of file diff --git a/tests/threads_unit.cpp b/tests/threads_unit.cpp new file mode 100644 index 000000000..b391f27ef --- /dev/null +++ b/tests/threads_unit.cpp @@ -0,0 +1,64 @@ +#include +#include +#include +#include +#include + +// We are mostly running this test to check for data races suing thread sanitizer. +void run(roaring_bitmap_t **rarray) { + int answer = 0; + for(size_t i = 0; i < 100; i++) { + roaring_bitmap_t *r1 = roaring_bitmap_copy(rarray[0]); + roaring_bitmap_t *r2 = roaring_bitmap_copy(rarray[1]); + roaring_bitmap_t *r3 = roaring_bitmap_copy(rarray[2]); + roaring_bitmap_and_inplace(r1, r2); + roaring_bitmap_andnot_inplace(r1, r3); + answer += (int)roaring_bitmap_get_cardinality(r1); + roaring_bitmap_free(r1); + } +} + +bool run_threads_unit_tests() { + roaring_bitmap_t *r1 = roaring_bitmap_create(); + + for (uint32_t i = 0; i < 50000; i++) { + if (i != 300) { + roaring_bitmap_add(r1, 65536 + i); + } + } + for (uint32_t i = 50000; i < 150000; i++) { + if ((i%500) == 0) { + roaring_bitmap_add(r1, i); + } + } + for (uint32_t i = 150000; i < 200000; i++) { + if ((i%2) == 0) { + roaring_bitmap_add(r1, i); + } + } + + roaring_bitmap_set_copy_on_write(r1, true); + roaring_bitmap_run_optimize(r1); + roaring_bitmap_t *r2 = roaring_bitmap_of(5, 10010,10020,10030,10040,10050); + roaring_bitmap_set_copy_on_write(r2, true); + roaring_bitmap_t *r3 = roaring_bitmap_copy(r1); + roaring_bitmap_set_copy_on_write(r3, true); + + roaring_bitmap_t* rarray1[3] = {r1, r2, r3}; + roaring_bitmap_t* rarray2[3] = {r1, r2, r3}; + std::thread thread1(run,rarray1); + std::thread thread2(run,rarray2); + thread1.join(); + thread2.join(); + roaring_bitmap_free(r1); + roaring_bitmap_free(r2); + roaring_bitmap_free(r3); + return true; +} + +int main() { + roaring::misc::tellmeall(); + bool is_ok = run_threads_unit_tests(); + if(is_ok) { printf("code run completed.\n"); } + return is_ok ? EXIT_SUCCESS : EXIT_FAILURE; +} diff --git a/tools/cmake/FindOptions.cmake b/tools/cmake/FindOptions.cmake index e1d1a036e..f7b8f9675 100644 --- a/tools/cmake/FindOptions.cmake +++ b/tools/cmake/FindOptions.cmake @@ -12,9 +12,7 @@ if(ROARING_SANITIZE) append(CMAKE_SHARED_LINKER_FLAGS "-fuse-ld=gold") endif() endif() -if(ROARING_SANITIZE_THREADS) - set(ROARING_SANITIZE_FLAGS "-fsanitize=thread -fno-sanitize-recover=all") -endif() + if((NOT MSVC) AND ROARING_ARCH) set(OPT_FLAGS "-march=${ROARING_ARCH}") endif() From 986b98e6a6eadf3d33f7d0039915acee96f4526b Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Tue, 9 May 2023 18:27:17 -0400 Subject: [PATCH 07/13] Various fixes --- tests/threads_unit.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/threads_unit.cpp b/tests/threads_unit.cpp index b391f27ef..fd16afecd 100644 --- a/tests/threads_unit.cpp +++ b/tests/threads_unit.cpp @@ -6,14 +6,12 @@ // We are mostly running this test to check for data races suing thread sanitizer. void run(roaring_bitmap_t **rarray) { - int answer = 0; for(size_t i = 0; i < 100; i++) { roaring_bitmap_t *r1 = roaring_bitmap_copy(rarray[0]); roaring_bitmap_t *r2 = roaring_bitmap_copy(rarray[1]); roaring_bitmap_t *r3 = roaring_bitmap_copy(rarray[2]); roaring_bitmap_and_inplace(r1, r2); roaring_bitmap_andnot_inplace(r1, r3); - answer += (int)roaring_bitmap_get_cardinality(r1); roaring_bitmap_free(r1); } } From dd483269cad4e4f57d0d9681b43adc0956137b49 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Tue, 9 May 2023 20:39:26 -0400 Subject: [PATCH 08/13] Patching the counter. --- microbenchmarks/performancecounters/apple_arm_events.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/microbenchmarks/performancecounters/apple_arm_events.h b/microbenchmarks/performancecounters/apple_arm_events.h index 5ce147ee2..c9eeb8aea 100644 --- a/microbenchmarks/performancecounters/apple_arm_events.h +++ b/microbenchmarks/performancecounters/apple_arm_events.h @@ -874,11 +874,11 @@ struct AppleEvents { u64 counters_1[KPC_MAX_COUNTERS] = {0}; static constexpr usize ev_count = sizeof(profile_events) / sizeof(profile_events[0]); + bool init = false; + bool worked = false; - inline bool setup_performance_counters() { - static bool init = false; - static bool worked = false; + inline bool setup_performance_counters() { if (init) { return worked; } From 5b43f116e4ea2a5165627fb9a5c3d740e68b5a08 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Tue, 9 May 2023 20:40:26 -0400 Subject: [PATCH 09/13] fixing silly leak --- tests/threads_unit.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/threads_unit.cpp b/tests/threads_unit.cpp index fd16afecd..2d913889c 100644 --- a/tests/threads_unit.cpp +++ b/tests/threads_unit.cpp @@ -13,6 +13,8 @@ void run(roaring_bitmap_t **rarray) { roaring_bitmap_and_inplace(r1, r2); roaring_bitmap_andnot_inplace(r1, r3); roaring_bitmap_free(r1); + roaring_bitmap_free(r2); + roaring_bitmap_free(r3); } } From 4767f9b7d6a9a3bb849a98b7da89e11ca12c9730 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Tue, 9 May 2023 22:08:52 -0400 Subject: [PATCH 10/13] Visual Studio does not support C11 atomics. --- README.md | 9 +++++++++ tests/CMakeLists.txt | 39 ++++++++++++++++++++++----------------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 26cef1d6e..fe51391a9 100644 --- a/README.md +++ b/README.md @@ -698,6 +698,15 @@ Our AVX-512 code is only enabled on recent hardware (Intel Ice Lake or better an Like, for example, STL containers or Java's default data structures, the CRoaring library has no built-in thread support. Thus whenever you modify a bitmap in one thread, it is unsafe to query it in others. It is safe however to query bitmaps (without modifying them) from several distinct threads, as long as you do not use the copy-on-write attribute. For example, you can safely copy a bitmap and use both copies in concurrently. One should probably avoid the use of the copy-on-write attribute in a threaded environment. +Some of our users rely on "copy-on-write" (default to disabled). A bitmap with the copy-on-write flag +set to true might generate shared containers. A shared container is just a reference to a single +container with reference counting (we keep track of the number of shallow copies). If you copy shared +containers over several threads, this might be unsafe due to the need to update the counter concurrently. +Thus for shared containers, we use reference counting with an atomic counter. If the library is compiled +as a C library (the default), we use C11 atomics. Unfortunately, Visual Studio does not support C11 +atomics at this times (though this is subject to change). Hence it is unsafe to copy shared containers +over multiple threads under Visual Studio. But it is safe in other C11-compliant compilers. + # How to best aggregate bitmaps? diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d7fd398ba..a1c9949ef 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -20,24 +20,29 @@ add_c_test(format_portability_unit) add_c_test(robust_deserialization_unit) add_c_test(container_comparison_unit) add_c_test(add_offset) -find_package(Threads) -if(Threads_FOUND) - message(STATUS "Your system supports threads.") - add_executable(threads_unit threads_unit.cpp) - target_link_libraries(threads_unit PRIVATE roaring Threads::Threads) - if(ROARING_SANITIZE_THREADS) - # libtsan might be needed - if(CMAKE_SYSTEM_NAME STREQUAL "Linux") - message(STATUS "Under Linux, you may need to install libtsan." ) +if(CMAKE_GENERATOR MATCHES "Visual Studio") + message(STATUS "Visual Studio does not yet support C11 atomics: thread-safety tests disabled.") + message(STATUS "https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/") +else(CMAKE_GENERATOR MATCHES "Visual Studio") + find_package(Threads) + if(Threads_FOUND) + message(STATUS "Your system supports threads.") + add_executable(threads_unit threads_unit.cpp) + target_link_libraries(threads_unit PRIVATE roaring Threads::Threads) + if(ROARING_SANITIZE_THREADS) + # libtsan might be needed + if(CMAKE_SYSTEM_NAME STREQUAL "Linux") + message(STATUS "Under Linux, you may need to install libtsan." ) + endif() + target_compile_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) + target_link_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) + message(STATUS "Sanitizing threads.") endif() - target_compile_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) - target_link_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) - message(STATUS "Sanitizing threads.") - endif() - add_test(threads_unit threads_unit) -else(Threads_FOUND) - message(STATUS "Your system does not support threads.") -endif(Threads_FOUND) + add_test(threads_unit threads_unit) + else(Threads_FOUND) + message(STATUS "Your system does not support threads.") + endif(Threads_FOUND) +endif(CMAKE_GENERATOR MATCHES "Visual Studio") if (NOT WIN32) # We exclude POSIX tests from Microsoft Windows From 25d41b37dfe55f147979d06286475ed452828eec Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Wed, 10 May 2023 17:39:57 -0400 Subject: [PATCH 11/13] adding vs atomics (#474) * Using Windows intrinsics. * Some fixes. * Removing silly comment. --- README.md | 4 ++-- include/roaring/portability.h | 19 ++++++++++------- src/containers/containers.c | 10 +++++++++ tests/CMakeLists.txt | 39 +++++++++++++++++------------------ 4 files changed, 43 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index fe51391a9..7db6b0335 100644 --- a/README.md +++ b/README.md @@ -704,8 +704,8 @@ container with reference counting (we keep track of the number of shallow copies containers over several threads, this might be unsafe due to the need to update the counter concurrently. Thus for shared containers, we use reference counting with an atomic counter. If the library is compiled as a C library (the default), we use C11 atomics. Unfortunately, Visual Studio does not support C11 -atomics at this times (though this is subject to change). Hence it is unsafe to copy shared containers -over multiple threads under Visual Studio. But it is safe in other C11-compliant compilers. +atomics at this times (though this is subject to change). To compensate, we +use Windows-specific code in such instances (`_InterlockedDecrement` `_InterlockedIncrement`). # How to best aggregate bitmaps? diff --git a/include/roaring/portability.h b/include/roaring/portability.h index 69ff57400..af682c76d 100644 --- a/include/roaring/portability.h +++ b/include/roaring/portability.h @@ -70,11 +70,6 @@ extern "C" { // portability definitions are in global scope, not a namespace #endif -#if CROARING_REGULAR_VISUAL_STUDIO && !defined(_WIN64) && !defined(CROARING_ACK_32BIT) -#pragma message( \ - "You appear to be attempting a 32-bit build under Visual Studio. We recommend a 64-bit build instead.") -#endif - #if defined(__SIZEOF_LONG_LONG__) && __SIZEOF_LONG_LONG__ != 8 #error This code assumes 64-bit long longs (by use of the GCC intrinsics). Your system is not currently supported. #endif @@ -404,28 +399,38 @@ static inline int roaring_hamming(uint64_t x) { #endif // __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ #endif - #if defined(__cplusplus) && __cplusplus >= 201103L #ifdef __has_include #if __has_include() // It is now safe: #define CROARING_CPP_ATOMIC 1 #define CROARING_C_ATOMIC 0 +#define CROARING_CPP_WINDOWS_ATOMIC 0 #include #endif //__has_include() #else // We lack __has_include to check: #define CROARING_CPP_ATOMIC 1 #define CROARING_C_ATOMIC 0 +#define CROARING_CPP_WINDOWS_ATOMIC 0 #include #endif //__has_include -#elif defined(__STDC_NO_ATOMICS__) || CROARING_REGULAR_VISUAL_STUDIO +#elif CROARING_REGULAR_VISUAL_STUDIO // https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/ #define CROARING_ATOMIC 0 #define CROARING_CPP_ATOMIC 0 +#define CROARING_CPP_WINDOWS_ATOMIC 1 +# include +# pragma intrinsic (_InterlockedIncrement) +# pragma intrinsic (_InterlockedDecrement) +#elif defined(__STDC_NO_ATOMICS__) +#define CROARING_ATOMIC 0 +#define CROARING_CPP_ATOMIC 0 +#define CROARING_CPP_WINDOWS_ATOMIC 0 #else // C #define CROARING_C_ATOMIC 1 #define CROARING_CPP_ATOMIC 0 +#define CROARING_CPP_WINDOWS_ATOMIC 0 #include #endif diff --git a/src/containers/containers.c b/src/containers/containers.c index 1667a1448..35b8d5635 100644 --- a/src/containers/containers.c +++ b/src/containers/containers.c @@ -141,6 +141,8 @@ container_t *get_copy_of_container( atomic_fetch_add(&(shared_container->counter), 1); #elif CROARING_CPP_ATOMIC std::atomic_fetch_add(&(shared_container->counter), 1); +#elif CROARING_CPP_WINDOWS_ATOMIC + _InterlockedIncrement(&(shared_container->counter)); #else shared_container->counter += 1; #endif @@ -155,6 +157,10 @@ container_t *get_copy_of_container( shared_container->container = c; shared_container->typecode = *typecode; + // At this point, we are creating new shared container + // so there should be no other references, and setting + // the counter to 2 is safe as long as the value + // is set before the return statement. #if CROARING_C_ATOMIC atomic_store(&(shared_container->counter), 2); #elif CROARING_CPP_ATOMIC @@ -206,6 +212,8 @@ container_t *shared_container_extract_copy( if(atomic_fetch_sub(&(sc->counter), 1) == 1) { #elif CROARING_CPP_ATOMIC if(std::atomic_fetch_sub(&(sc->counter), 1) == 1) { +#elif CROARING_CPP_WINDOWS_ATOMIC + if(_InterlockedDecrement(&(sc->counter)) == 0) { #else assert(sc->counter > 0); sc->counter--; @@ -226,6 +234,8 @@ void shared_container_free(shared_container_t *container) { if(atomic_fetch_sub(&(container->counter), 1) == 1) { #elif CROARING_CPP_ATOMIC if(std::atomic_fetch_sub(&(container->counter), 1) == 1) { +#elif CROARING_CPP_WINDOWS_ATOMIC + if(_InterlockedDecrement(&(container->counter)) == 0) { #else assert(container->counter > 0); container->counter--; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a1c9949ef..27e826168 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -21,28 +21,27 @@ add_c_test(robust_deserialization_unit) add_c_test(container_comparison_unit) add_c_test(add_offset) if(CMAKE_GENERATOR MATCHES "Visual Studio") - message(STATUS "Visual Studio does not yet support C11 atomics: thread-safety tests disabled.") + message(STATUS "Visual Studio does not yet support C11 atomics.") message(STATUS "https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/") -else(CMAKE_GENERATOR MATCHES "Visual Studio") - find_package(Threads) - if(Threads_FOUND) - message(STATUS "Your system supports threads.") - add_executable(threads_unit threads_unit.cpp) - target_link_libraries(threads_unit PRIVATE roaring Threads::Threads) - if(ROARING_SANITIZE_THREADS) - # libtsan might be needed - if(CMAKE_SYSTEM_NAME STREQUAL "Linux") - message(STATUS "Under Linux, you may need to install libtsan." ) - endif() - target_compile_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) - target_link_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) - message(STATUS "Sanitizing threads.") +endif() +find_package(Threads) +if(Threads_FOUND) + message(STATUS "Your system supports threads.") + add_executable(threads_unit threads_unit.cpp) + target_link_libraries(threads_unit PRIVATE roaring Threads::Threads) + if(ROARING_SANITIZE_THREADS) + # libtsan might be needed + if(CMAKE_SYSTEM_NAME STREQUAL "Linux") + message(STATUS "Under Linux, you may need to install libtsan." ) endif() - add_test(threads_unit threads_unit) - else(Threads_FOUND) - message(STATUS "Your system does not support threads.") - endif(Threads_FOUND) -endif(CMAKE_GENERATOR MATCHES "Visual Studio") + target_compile_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) + target_link_options(threads_unit PRIVATE -fsanitize=thread -fno-sanitize-recover=all) + message(STATUS "Sanitizing threads.") + endif() + add_test(threads_unit threads_unit) +else(Threads_FOUND) + message(STATUS "Your system does not support threads.") +endif(Threads_FOUND) if (NOT WIN32) # We exclude POSIX tests from Microsoft Windows From f91b396eb007af697960680d5560bfb1038e7e51 Mon Sep 17 00:00:00 2001 From: Zachary Dremann Date: Fri, 12 May 2023 17:13:36 -0400 Subject: [PATCH 12/13] Use typedef/inline functions to centralize atomic ref count ops (#475) * Use typedef/inline functions to centralize atomic ref count ops * __STDC_NO_ATOMICS__ only matters if we're c11+ I _think_ this will fix all the msvc compile issues * MSVC can probably use Interlocked functions even in c++ * _Interlocked ops actually take a signed argument * Ack. uint32->uint32_t Probably was rust brain, thinking `u32` --- include/roaring/containers/containers.h | 16 +-- include/roaring/portability.h | 153 +++++++++++++++++++----- src/containers/containers.c | 44 +------ src/isadetection.c | 10 +- src/roaring.c | 17 +-- 5 files changed, 135 insertions(+), 105 deletions(-) diff --git a/include/roaring/containers/containers.h b/include/roaring/containers/containers.h index b25d10866..b10629644 100644 --- a/include/roaring/containers/containers.h +++ b/include/roaring/containers/containers.h @@ -55,25 +55,11 @@ extern "C" { namespace roaring { namespace internal { * A shared container is a wrapper around a container * with reference counting. */ -#if CROARING_C_ATOMIC STRUCT_CONTAINER(shared_container_s) { container_t *container; uint8_t typecode; - _Atomic(uint32_t) counter; // to be managed atomically + croaring_refcount_t counter; // to be managed atomically }; -#elif CROARING_CPP_ATOMIC -STRUCT_CONTAINER(shared_container_s) { - container_t *container; - uint8_t typecode; - std::atomic counter; // to be managed atomically -}; -#else -STRUCT_CONTAINER(shared_container_s) { - container_t *container; - uint8_t typecode; - uint32_t counter; // to be managed atomically -}; -#endif typedef struct shared_container_s shared_container_t; diff --git a/include/roaring/portability.h b/include/roaring/portability.h index af682c76d..ebfa55d73 100644 --- a/include/roaring/portability.h +++ b/include/roaring/portability.h @@ -370,7 +370,7 @@ static inline int roaring_hamming(uint64_t x) { #if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) #define CROARING_IS_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__) - #elif defined(_WIN32) +#elif defined(_WIN32) #define CROARING_IS_BIG_ENDIAN 0 #else #if defined(__APPLE__) || defined(__FreeBSD__) // defined __BYTE_ORDER__ && defined __ORDER_BIG_ENDIAN__ @@ -399,41 +399,130 @@ static inline int roaring_hamming(uint64_t x) { #endif // __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ #endif -#if defined(__cplusplus) && __cplusplus >= 201103L -#ifdef __has_include -#if __has_include() -// It is now safe: -#define CROARING_CPP_ATOMIC 1 -#define CROARING_C_ATOMIC 0 -#define CROARING_CPP_WINDOWS_ATOMIC 0 +// Defines for the possible CROARING atomic implementations +#define CROARING_ATOMIC_IMPL_NONE 1 +#define CROARING_ATOMIC_IMPL_CPP 2 +#define CROARING_ATOMIC_IMPL_C 3 +#define CROARING_ATOMIC_IMPL_C_WINDOWS 4 + +// If the use has forced a specific implementation, use that, otherwise, +// figure out the best implementation we can use. +#if !defined(CROARING_ATOMIC_IMPL) + #if defined(__cplusplus) && __cplusplus >= 201103L + #ifdef __has_include + #if __has_include() + #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_CPP + #endif //__has_include() + #else + // We lack __has_include to check: + #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_CPP + #endif //__has_include + #elif __STDC_VERSION__ >= 201112L && !defined(__STDC_NO_ATOMICS__) + #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_C + #elif CROARING_REGULAR_VISUAL_STUDIO + // https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/ + #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_C_WINDOWS + #endif +#endif // !defined(CROARING_ATOMIC_IMPL) + +#if !defined(CROARING_ATOMIC_IMPL) + #pragma message ( "No atomic implementation found, copy on write bitmaps will not be threadsafe" ) + #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_NONE +#endif + +#if CROARING_ATOMIC_IMPL == CROARING_ATOMIC_IMPL_C +#include +typedef _Atomic(uint32_t) croaring_refcount_t; + +static inline void croaring_refcount_inc(croaring_refcount_t *val) { + // Increasing the reference counter can always be done with + // memory_order_relaxed: New references to an object can only be formed from + // an existing reference, and passing an existing reference from one thread to + // another must already provide any required synchronization. + atomic_fetch_add_explicit(val, 1, memory_order_relaxed); +} + +static inline bool croaring_refcount_dec(croaring_refcount_t *val) { + // It is important to enforce any possible access to the object in one thread + // (through an existing reference) to happen before deleting the object in a + // different thread. This is achieved by a "release" operation after dropping + // a reference (any access to the object through this reference must obviously + // happened before), and an "acquire" operation before deleting the object. + bool is_zero = atomic_fetch_sub_explicit(val, 1, memory_order_release) == 1; + if (is_zero) { + atomic_thread_fence(memory_order_acquire); + } + return is_zero; +} + +static inline uint32_t croaring_refcount_get(croaring_refcount_t *val) { + return atomic_load_explicit(val, memory_order_relaxed); +} +#elif CROARING_ATOMIC_IMPL == CROARING_ATOMIC_IMPL_CPP #include -#endif //__has_include() +typedef std::atomic croaring_refcount_t; + +static inline void croaring_refcount_inc(croaring_refcount_t *val) { + val->fetch_add(1, std::memory_order_relaxed); +} + +static inline bool croaring_refcount_dec(croaring_refcount_t *val) { + // See above comments on the c11 atomic implementation for memory ordering + bool is_zero = val->fetch_sub(1, std::memory_order_release) == 1; + if (is_zero) { + std::atomic_thread_fence(std::memory_order_acquire); + } + return is_zero; +} + +static inline uint32_t croaring_refcount_get(croaring_refcount_t *val) { + return val->load(std::memory_order_relaxed); +} +#elif CROARING_ATOMIC_IMPL == CROARING_ATOMIC_IMPL_C_WINDOWS +#include +#pragma intrinsic(_InterlockedIncrement) +#pragma intrinsic(_InterlockedDecrement) + +// _InterlockedIncrement and _InterlockedDecrement take a (signed) long, and +// overflow is defined to wrap, so we can pretend it is a uint32_t for our case +typedef volatile long croaring_refcount_t; + +static inline void croaring_refcount_inc(croaring_refcount_t *val) { + _InterlockedIncrement(val); +} + +static inline bool croaring_refcount_dec(croaring_refcount_t *val) { + return _InterlockedDecrement(val) == 0; +} + +static inline uint32_t croaring_refcount_get(croaring_refcount_t *val) { + // Per https://learn.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access + // > Simple reads and writes to properly-aligned 32-bit variables are atomic + // > operations. In other words, you will not end up with only one portion + // > of the variable updated; all bits are updated in an atomic fashion. + return *val; +} +#elif CROARING_ATOMIC_IMPL == CROARING_ATOMIC_IMPL_NONE +typedef uint32_t croaring_refcount_t; + +static inline void croaring_refcount_inc(croaring_refcount_t *val) { + *val += 1; +} + +static inline bool croaring_refcount_dec(croaring_refcount_t *val) { + assert(*val > 0); + *val -= 1; + return val == 0; +} + +static inline uint32_t croaring_refcount_get(croaring_refcount_t *val) { + return *val; +} #else -// We lack __has_include to check: -#define CROARING_CPP_ATOMIC 1 -#define CROARING_C_ATOMIC 0 -#define CROARING_CPP_WINDOWS_ATOMIC 0 -#include -#endif //__has_include -#elif CROARING_REGULAR_VISUAL_STUDIO -// https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/ -#define CROARING_ATOMIC 0 -#define CROARING_CPP_ATOMIC 0 -#define CROARING_CPP_WINDOWS_ATOMIC 1 -# include -# pragma intrinsic (_InterlockedIncrement) -# pragma intrinsic (_InterlockedDecrement) -#elif defined(__STDC_NO_ATOMICS__) -#define CROARING_ATOMIC 0 -#define CROARING_CPP_ATOMIC 0 -#define CROARING_CPP_WINDOWS_ATOMIC 0 -#else // C -#define CROARING_C_ATOMIC 1 -#define CROARING_CPP_ATOMIC 0 -#define CROARING_CPP_WINDOWS_ATOMIC 0 -#include +#error "Unknown atomic implementation" #endif + // We need portability.h to be included first, // but we also always want isadetection.h to be // included (right after). diff --git a/src/containers/containers.c b/src/containers/containers.c index 35b8d5635..9bbd758c8 100644 --- a/src/containers/containers.c +++ b/src/containers/containers.c @@ -137,15 +137,7 @@ container_t *get_copy_of_container( shared_container_t *shared_container; if (*typecode == SHARED_CONTAINER_TYPE) { shared_container = CAST_shared(c); -#if CROARING_C_ATOMIC - atomic_fetch_add(&(shared_container->counter), 1); -#elif CROARING_CPP_ATOMIC - std::atomic_fetch_add(&(shared_container->counter), 1); -#elif CROARING_CPP_WINDOWS_ATOMIC - _InterlockedIncrement(&(shared_container->counter)); -#else - shared_container->counter += 1; -#endif + croaring_refcount_inc(&shared_container->counter); return shared_container; } assert(*typecode != SHARED_CONTAINER_TYPE); @@ -159,15 +151,9 @@ container_t *get_copy_of_container( shared_container->typecode = *typecode; // At this point, we are creating new shared container // so there should be no other references, and setting - // the counter to 2 is safe as long as the value - // is set before the return statement. -#if CROARING_C_ATOMIC - atomic_store(&(shared_container->counter), 2); -#elif CROARING_CPP_ATOMIC - std::atomic_store(&(shared_container->counter), 2); -#else + // the counter to 2 - even non-atomically - is safe as + // long as the value is set before the return statement. shared_container->counter = 2; -#endif *typecode = SHARED_CONTAINER_TYPE; return shared_container; @@ -208,17 +194,7 @@ container_t *shared_container_extract_copy( assert(sc->typecode != SHARED_CONTAINER_TYPE); *typecode = sc->typecode; container_t *answer; -#if CROARING_C_ATOMIC - if(atomic_fetch_sub(&(sc->counter), 1) == 1) { -#elif CROARING_CPP_ATOMIC - if(std::atomic_fetch_sub(&(sc->counter), 1) == 1) { -#elif CROARING_CPP_WINDOWS_ATOMIC - if(_InterlockedDecrement(&(sc->counter)) == 0) { -#else - assert(sc->counter > 0); - sc->counter--; - if (sc->counter == 0) { -#endif + if (croaring_refcount_dec(&sc->counter)) { answer = sc->container; sc->container = NULL; // paranoid roaring_free(sc); @@ -230,17 +206,7 @@ container_t *shared_container_extract_copy( } void shared_container_free(shared_container_t *container) { -#if CROARING_C_ATOMIC - if(atomic_fetch_sub(&(container->counter), 1) == 1) { -#elif CROARING_CPP_ATOMIC - if(std::atomic_fetch_sub(&(container->counter), 1) == 1) { -#elif CROARING_CPP_WINDOWS_ATOMIC - if(_InterlockedDecrement(&(container->counter)) == 0) { -#else - assert(container->counter > 0); - container->counter--; - if (container->counter == 0) { -#endif + if (croaring_refcount_dec(&container->counter)) { assert(container->typecode != SHARED_CONTAINER_TYPE); container_free(container->container, container->typecode); container->container = NULL; // paranoid diff --git a/src/isadetection.c b/src/isadetection.c index c611a0e94..feb3abef4 100644 --- a/src/isadetection.c +++ b/src/isadetection.c @@ -194,14 +194,14 @@ static inline uint32_t dynamic_croaring_detect_supported_architectures() { #if defined(__x86_64__) || defined(_M_AMD64) // x64 -#if defined(__cplusplus) +#if CROARING_ATOMIC_IMPL == CROARING_ATOMIC_IMPL_CPP static inline uint32_t croaring_detect_supported_architectures() { // thread-safe as per the C++11 standard. static uint32_t buffer = dynamic_croaring_detect_supported_architectures(); return buffer; } -#elif CROARING_C_ATOMIC -uint32_t croaring_detect_supported_architectures() { +#elif CROARING_ATOMIC_IMPL == CROARING_ATOMIC_IMPL_C +static uint32_t croaring_detect_supported_architectures() { // we use an atomic for thread safety static _Atomic uint32_t buffer = CROARING_UNINITIALIZED; if (buffer == CROARING_UNINITIALIZED) { @@ -211,9 +211,9 @@ uint32_t croaring_detect_supported_architectures() { return buffer; } #else -// If we do not have C11 atomics, we do the best we can. +// If we do not have atomics, we do the best we can. static inline uint32_t croaring_detect_supported_architectures() { - static int buffer = CROARING_UNINITIALIZED; + static uint32_t buffer = CROARING_UNINITIALIZED; if (buffer == CROARING_UNINITIALIZED) { buffer = dynamic_croaring_detect_supported_architectures(); } diff --git a/src/roaring.c b/src/roaring.c index 631f2c7fe..ff6843820 100644 --- a/src/roaring.c +++ b/src/roaring.c @@ -348,20 +348,9 @@ void roaring_bitmap_printf_describe(const roaring_bitmap_t *r) { get_full_container_name(ra->containers[i], ra->typecodes[i]), container_get_cardinality(ra->containers[i], ra->typecodes[i])); if (ra->typecodes[i] == SHARED_CONTAINER_TYPE) { -#if CROARING_C_ATOMIC - printf( - "(shared count = %" PRIu32 " )", - atomic_load(&(CAST_shared(ra->containers[i])->counter))); -#elif CROARING_CPP_ATOMIC - printf( - "(shared count = %" PRIu32 " )", - std::atomic_load(&(CAST_shared(ra->containers[i])->counter))); -#else - printf( - "(shared count = %" PRIu32 " )", - CAST_shared(ra->containers[i])->counter); -#endif - + printf("(shared count = %" PRIu32 " )", + croaring_refcount_get( + &(CAST_shared(ra->containers[i])->counter))); } if (i + 1 < ra->size) { From 77e16bd58a089ee630b3dd2be53dc33bd630ad82 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Fri, 12 May 2023 22:38:58 -0400 Subject: [PATCH 13/13] Minor fixes --- include/roaring/portability.h | 4 ++-- tests/CMakeLists.txt | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/include/roaring/portability.h b/include/roaring/portability.h index ebfa55d73..1dd6b04b1 100644 --- a/include/roaring/portability.h +++ b/include/roaring/portability.h @@ -414,8 +414,8 @@ static inline int roaring_hamming(uint64_t x) { #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_CPP #endif //__has_include() #else - // We lack __has_include to check: - #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_CPP + // We lack __has_include to check: + #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_CPP #endif //__has_include #elif __STDC_VERSION__ >= 201112L && !defined(__STDC_NO_ATOMICS__) #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_C diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 27e826168..d7fd398ba 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -20,10 +20,6 @@ add_c_test(format_portability_unit) add_c_test(robust_deserialization_unit) add_c_test(container_comparison_unit) add_c_test(add_offset) -if(CMAKE_GENERATOR MATCHES "Visual Studio") - message(STATUS "Visual Studio does not yet support C11 atomics.") - message(STATUS "https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/") -endif() find_package(Threads) if(Threads_FOUND) message(STATUS "Your system supports threads.")