From a9bcec02fb8c08e9ab3b000df81e162f28554487 Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 11:12:03 -0400 Subject: [PATCH 1/9] fix for a potential race condition --- similarity_search/include/thread_pool.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/similarity_search/include/thread_pool.h b/similarity_search/include/thread_pool.h index 21c0b34..d953b15 100644 --- a/similarity_search/include/thread_pool.h +++ b/similarity_search/include/thread_pool.h @@ -68,6 +68,7 @@ namespace similarity { // keep track of exceptions in threads // https://stackoverflow.com/a/32428427/1713196 std::exception_ptr lastException = nullptr; + std::mutex lastExceptMutex; for (int i = 0; i < numThreads; ++i) { threads.push_back(std::thread([&] { @@ -80,6 +81,7 @@ namespace similarity { try { fn(id); } catch (...) { + std::unique_lock lastExcepLock(lastExceptMutex); lastException = std::current_exception(); } } From 6001be55a6c9c8639fdfd7b638cdcac279e37f9d Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 11:49:14 -0400 Subject: [PATCH 2/9] preventing race --- similarity_search/include/thread_pool.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/similarity_search/include/thread_pool.h b/similarity_search/include/thread_pool.h index d953b15..d1fdee0 100644 --- a/similarity_search/include/thread_pool.h +++ b/similarity_search/include/thread_pool.h @@ -74,8 +74,12 @@ namespace similarity { threads.push_back(std::thread([&] { while (true) { int id = current.fetch_add(1); - if ((id >= final) || lastException) { - break; + + { + std::unique_lock lastExcepLock(lastExceptMutex); + if ((id >= final) || lastException) { + break; + } } try { From 8ace97cd2dd3342fe25c082684e8b27986639ea1 Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 12:10:50 -0400 Subject: [PATCH 3/9] more fixes for a race condition in ParallelFor --- similarity_search/include/thread_pool.h | 16 +++++++--------- similarity_search/src/method/hnsw.cc | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/similarity_search/include/thread_pool.h b/similarity_search/include/thread_pool.h index d1fdee0..88689c5 100644 --- a/similarity_search/include/thread_pool.h +++ b/similarity_search/include/thread_pool.h @@ -57,29 +57,26 @@ namespace similarity { // replacement for the openmp '#pragma omp parallel for' directive // only handles a subset of functionality (no reductions etc) template - inline void ParallelFor(int initial, int final, int numThreads, Function fn) { + inline void ParallelFor(size_t initial, size_t finalId, size_t numThreads, Function fn) { if (numThreads <= 0) { numThreads = std::thread::hardware_concurrency(); } - std::vector threads; - std::atomic current(initial); + std::vector threads; + std::atomic current(initial); // keep track of exceptions in threads // https://stackoverflow.com/a/32428427/1713196 std::exception_ptr lastException = nullptr; std::mutex lastExceptMutex; - for (int i = 0; i < numThreads; ++i) { + for (size_t i = 0; i < numThreads; ++i) { threads.push_back(std::thread([&] { while (true) { int id = current.fetch_add(1); - { - std::unique_lock lastExcepLock(lastExceptMutex); - if ((id >= final) || lastException) { - break; - } + if ((id >= finalId)) { + break; } try { @@ -87,6 +84,7 @@ namespace similarity { } catch (...) { std::unique_lock lastExcepLock(lastExceptMutex); lastException = std::current_exception(); + break; } } })); diff --git a/similarity_search/src/method/hnsw.cc b/similarity_search/src/method/hnsw.cc index 9bd9229..bb8009a 100644 --- a/similarity_search/src/method/hnsw.cc +++ b/similarity_search/src/method/hnsw.cc @@ -1012,7 +1012,7 @@ namespace similarity { ++currElem; } - for (int_fast32_t i = 0; i < query->GetK() && i < sortedArr.size(); ++i) { + for (uint_fast32_t i = 0; i < query->GetK() && i < sortedArr.size(); ++i) { query->CheckAndAddToResult(queueData[i].key, queueData[i].data->getData()); } From 687a7bf9e3c7554c9a967ec7a3927618f0e99c9c Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 12:30:00 -0400 Subject: [PATCH 4/9] int -> size_t --- similarity_search/include/thread_pool.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/similarity_search/include/thread_pool.h b/similarity_search/include/thread_pool.h index 88689c5..4a36c51 100644 --- a/similarity_search/include/thread_pool.h +++ b/similarity_search/include/thread_pool.h @@ -73,7 +73,7 @@ namespace similarity { for (size_t i = 0; i < numThreads; ++i) { threads.push_back(std::thread([&] { while (true) { - int id = current.fetch_add(1); + size_t id = current.fetch_add(1); if ((id >= finalId)) { break; From 57ae625a250f63efc08be8422c13e9d485e932a0 Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 13:38:32 -0400 Subject: [PATCH 5/9] previously introduced bug: the exception won't be noticed until other threads are finished. --- similarity_search/include/thread_pool.h | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/similarity_search/include/thread_pool.h b/similarity_search/include/thread_pool.h index 4a36c51..cf89fee 100644 --- a/similarity_search/include/thread_pool.h +++ b/similarity_search/include/thread_pool.h @@ -54,16 +54,19 @@ namespace similarity { */ - // replacement for the openmp '#pragma omp parallel for' directive - // only handles a subset of functionality (no reductions etc) + /* + * replacement for the openmp '#pragma omp parallel for' directive + * only handles a subset of functionality (no reductions etc) + * Process ids from start (inclusive) to end (EXCLUSIVE) + */ template - inline void ParallelFor(size_t initial, size_t finalId, size_t numThreads, Function fn) { + inline void ParallelFor(size_t start, size_t end, size_t numThreads, Function fn) { if (numThreads <= 0) { numThreads = std::thread::hardware_concurrency(); } std::vector threads; - std::atomic current(initial); + std::atomic current(start); // keep track of exceptions in threads // https://stackoverflow.com/a/32428427/1713196 @@ -75,7 +78,7 @@ namespace similarity { while (true) { size_t id = current.fetch_add(1); - if ((id >= finalId)) { + if ((id >= end)) { break; } @@ -84,6 +87,13 @@ namespace similarity { } catch (...) { std::unique_lock lastExcepLock(lastExceptMutex); lastException = std::current_exception(); + /* + * This will work even when current is the largest value that + * size_t can fit, because feat_add returns the previous value + * before the increment (what will result in overflow + * and produce 0 instead of current + 1). + */ + current = end; break; } } From 2cd3b0721e807ae7dda041674858db29b40666df Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 14:18:06 -0400 Subject: [PATCH 6/9] fixing a comment --- similarity_search/include/thread_pool.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/similarity_search/include/thread_pool.h b/similarity_search/include/thread_pool.h index cf89fee..0a98141 100644 --- a/similarity_search/include/thread_pool.h +++ b/similarity_search/include/thread_pool.h @@ -89,7 +89,7 @@ namespace similarity { lastException = std::current_exception(); /* * This will work even when current is the largest value that - * size_t can fit, because feat_add returns the previous value + * size_t can fit, because fetch_add returns the previous value * before the increment (what will result in overflow * and produce 0 instead of current + 1). */ From 36c7c06d069776b932a6dc6ff5fec24eb574c17f Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 16:54:53 -0400 Subject: [PATCH 7/9] Replacing an obsolete option for ICC --- similarity_search/CMakeLists.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/similarity_search/CMakeLists.txt b/similarity_search/CMakeLists.txt index fae4e53..529bec1 100644 --- a/similarity_search/CMakeLists.txt +++ b/similarity_search/CMakeLists.txt @@ -51,8 +51,8 @@ elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Intel") if (CXX_COMPILER_VERSION VERSION_LESS 14.0.1) message(FATAL_ERROR "Intel version must be at least 14.0.1!") endif() - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -openmp -march=native -fpic") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -ggdb -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -openmp -march=native -fpic") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -qopenmp -march=native -fpic") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -ggdb -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -qopenmp -march=native -fpic") elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang") if (CXX_COMPILER_VERSION VERSION_LESS 4.2.1) message(FATAL_ERROR "Clang version must be at least 3.4 (GCC >= 4.2.1 equivalent)!") From 765b3b1037ecec6c6274da2a439a09ecb8ca5702 Mon Sep 17 00:00:00 2001 From: searchivairus Date: Fri, 21 Jul 2017 20:00:56 -0400 Subject: [PATCH 8/9] Further fixes for the make --- similarity_search/CMakeLists.txt | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/similarity_search/CMakeLists.txt b/similarity_search/CMakeLists.txt index 529bec1..4064296 100644 --- a/similarity_search/CMakeLists.txt +++ b/similarity_search/CMakeLists.txt @@ -42,17 +42,17 @@ if(${CMAKE_CXX_COMPILER_ID} STREQUAL "GNU") message(FATAL_ERROR "GCC version must be at least 4.7!") endif() # Uncomment the following lines to see how the code compiles without AVX,SSE4.2 and/or SSE2 - #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fopenmp -fpic -march=x86-64") - #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fopenmp -fpic -march=core2") - #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fopenmp -fpic -msse4.2") - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -Ofast -lm -lrt -DNDEBUG -std=c++11 -fopenmp -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -lm -lrt -DNDEBUG -std=c++11 -fopenmp -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") + #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=x86-64") + #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=core2") + #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -msse4.2") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Intel") if (CXX_COMPILER_VERSION VERSION_LESS 14.0.1) message(FATAL_ERROR "Intel version must be at least 14.0.1!") endif() - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -qopenmp -march=native -fpic") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -ggdb -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -qopenmp -march=native -fpic") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -lpthread -march=native -fpic") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -ggdb -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -lpthread -march=native -fpic") elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang") if (CXX_COMPILER_VERSION VERSION_LESS 4.2.1) message(FATAL_ERROR "Clang version must be at least 3.4 (GCC >= 4.2.1 equivalent)!") @@ -62,8 +62,8 @@ elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang") set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -O3 -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=native") set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=native") else() - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -O3 -DNDEBUG -std=c++11 -DHAVE_CXX0X -fopenmp -march=native -fpic") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -DNDEBUG -std=c++11 -DHAVE_CXX0X -fopenmp -march=native -fpic") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -O3 -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -fpic") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -fpic") endif() #message(FATAL_ERROR "CLANG ${CMAKE_SYSTEM_NAME}") elseif(WIN32) From d571c7d951850ec8ac8bc9c6fc83ebfd8f68ad68 Mon Sep 17 00:00:00 2001 From: searchivairus Date: Sat, 22 Jul 2017 15:17:23 -0400 Subject: [PATCH 9/9] adding -pthread (needed because -fopenmp is deleted) --- similarity_search/CMakeLists.txt | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/similarity_search/CMakeLists.txt b/similarity_search/CMakeLists.txt index 4064296..1451b30 100644 --- a/similarity_search/CMakeLists.txt +++ b/similarity_search/CMakeLists.txt @@ -42,28 +42,28 @@ if(${CMAKE_CXX_COMPILER_ID} STREQUAL "GNU") message(FATAL_ERROR "GCC version must be at least 4.7!") endif() # Uncomment the following lines to see how the code compiles without AVX,SSE4.2 and/or SSE2 - #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=x86-64") - #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=core2") - #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -msse4.2") - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") + #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -fpic -march=x86-64") + #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -fpic -march=core2") + #set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lm -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -fpic -msse4.2") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -Ofast -lm -lrt -DNDEBUG -std=c++11 -pthread -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -lm -lrt -DNDEBUG -std=c++11 -pthread -DHAVE_CXX0X -march=native -Wl,--no-as-needed -fpic") elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Intel") if (CXX_COMPILER_VERSION VERSION_LESS 14.0.1) message(FATAL_ERROR "Intel version must be at least 14.0.1!") endif() - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -lpthread -march=native -fpic") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -ggdb -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -lpthread -march=native -fpic") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Ofast -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -march=native -fpic") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -ggdb -lrt -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -march=native -fpic") elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang") if (CXX_COMPILER_VERSION VERSION_LESS 4.2.1) message(FATAL_ERROR "Clang version must be at least 3.4 (GCC >= 4.2.1 equivalent)!") endif() if (CMAKE_SYSTEM_NAME MATCHES Darwin) # MACOSX - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -O3 -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=native") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -DNDEBUG -std=c++11 -DHAVE_CXX0X -fpic -march=native") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -O3 -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -fpic -march=native") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -fpic -march=native") else() - set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -O3 -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -fpic") - set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -DNDEBUG -std=c++11 -DHAVE_CXX0X -march=native -fpic") + set (CMAKE_CXX_FLAGS_RELEASE "-Wall -Wunreachable-code -Wcast-align -O3 -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -march=native -fpic") + set (CMAKE_CXX_FLAGS_DEBUG "-Wall -Wunreachable-code -Wcast-align -ggdb -DNDEBUG -std=c++11 -DHAVE_CXX0X -pthread -march=native -fpic") endif() #message(FATAL_ERROR "CLANG ${CMAKE_SYSTEM_NAME}") elseif(WIN32)