Skip to content

Commit

Permalink
Bugfix for mutex locking in the non-OpenMP serialize() default. (#23)
Browse files Browse the repository at this point in the history
Previously hidden because we only tested the path with the macro override.
Also commented on why we use an external function for the mutex definition.
  • Loading branch information
LTLA authored Aug 27, 2024
1 parent e4eecc8 commit 2eefc2d
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 11 deletions.
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
cmake_minimum_required(VERSION 3.24)

project(tatami_hdf5
VERSION 2.0.1
VERSION 2.0.2
DESCRIPTION "tatami bindings for HDF5"
LANGUAGES CXX)

Expand Down
18 changes: 9 additions & 9 deletions include/tatami_hdf5/serialize.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,22 @@
* @brief Default locking for serial access.
*/

/**
* @cond
*/
#ifndef TATAMI_HDF5_PARALLEL_LOCK
#include "subpar/subpar.hpp"
#ifndef SUBPAR_USES_OPENMP
#include <thread>
#include <mutex>
#include <thread>
namespace tatami_hdf5 {
// We put this in a separate function instead of a static function variable
// inside serialize(), as we would get a different mutex for each instance
// of serialize() created with a different Function_.
inline auto& get_default_hdf5_lock() {
static std::mutex hdf5_lock;
return hdf5_lock;
}
}
#endif
#endif
/**
* @endcond
*/

namespace tatami_hdf5 {

Expand All @@ -48,15 +45,18 @@ template<class Function_>
void serialize(Function_ f) {
#ifdef TATAMI_HDF5_PARALLEL_LOCK
TATAMI_HDF5_PARALLEL_LOCK(f);
#elif defined(SUBPAR_USES_OPENMP)
#else
#ifdef SUBPAR_USES_OPENMP
#pragma omp critical(hdf5)
{
f();
}
#else
std::lock_guard<std::mutex> thing(get_hdf5_lock());
auto& h5lock = get_default_hdf5_lock();
std::lock_guard<std::mutex> thing(h5lock);
f();
#endif
#endif
}

}
Expand Down
10 changes: 9 additions & 1 deletion tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ endmacro()

decorate_test(libtest)

add_executable(
cuslocktest
src/write_compressed_sparse_matrix.cpp
src/DenseMatrix.cpp
src/CompressedSparseMatrix.cpp
)
decorate_test(cuslocktest)
target_compile_definitions(cuslocktest PRIVATE TATAMI_HDF5_TEST_PARALLEL_ONLY=1 TATAMI_HDF5_TEST_CUSTOM_LOCK=1)

find_package(OpenMP)
if(OpenMP_FOUND)
add_executable(
Expand All @@ -59,7 +68,6 @@ if(OpenMP_FOUND)
src/DenseMatrix.cpp
src/CompressedSparseMatrix.cpp
)

decorate_test(omptest)
target_link_libraries(omptest OpenMP::OpenMP_CXX)
target_compile_definitions(omptest PRIVATE TATAMI_HDF5_TEST_PARALLEL_ONLY=1)
Expand Down
2 changes: 2 additions & 0 deletions tests/src/custom_parallel.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// OpenMP critical regions instead of using this mutex-based lock.

#ifndef _OPENMP
#ifdef TATAMI_HDF5_TEST_CUSTOM_LOCK
#include <thread>
#include <mutex>

Expand All @@ -24,3 +25,4 @@ void hdf5_serialize(Function f) {
#endif

#endif
#endif

0 comments on commit 2eefc2d

Please sign in to comment.