From 269f741a2ff112066350ba677e9c20416e01c36d Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Thu, 15 Jul 2021 17:47:59 -0700 Subject: [PATCH 01/12] Remove OpenMP from CMake build --- CMakeLists.txt | 1 - cmake/TreeliteConfig.cmake.in | 6 ------ src/CMakeLists.txt | 16 ---------------- src/annotator.cc | 1 - 4 files changed, 24 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 79e59cec..3e68802d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12,7 +12,6 @@ if(MSVC) endif() option(TEST_COVERAGE "C++ test coverage" OFF) -option(USE_OPENMP "Use OpenMP" ON) option(BUILD_CPP_TEST "Build C++ tests" OFF) option(BUILD_STATIC_LIBS "Build static libs, in addition to dynamic libs" OFF) option(DETECT_CONDA_ENV "Enable detection of conda environment for dependencies" ON) diff --git a/cmake/TreeliteConfig.cmake.in b/cmake/TreeliteConfig.cmake.in index bc5de108..b305a2ab 100644 --- a/cmake/TreeliteConfig.cmake.in +++ b/cmake/TreeliteConfig.cmake.in @@ -1,11 +1,5 @@ @PACKAGE_INIT@ -include(CMakeFindDependencyMacro) - -set(USE_OPENMP @USE_OPENMP@) -if(USE_OPENMP) - find_dependency(OpenMP) -endif() if(NOT TARGET treelite::treelite) include(${CMAKE_CURRENT_LIST_DIR}/TreeliteTargets.cmake) endif() diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index cee954ff..51a00e65 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -11,17 +11,6 @@ endif(UNIX) add_library(objtreelite_common OBJECT) # Component shared by both main package and runtime -if(USE_OPENMP) - if (APPLE) - # Require CMake 3.16+ on Mac OSX, as previous versions of CMake had trouble locating - # OpenMP on Mac. See https://github.com/dmlc/xgboost/pull/5146#issuecomment-568312706 - cmake_minimum_required(VERSION 3.16) - endif (APPLE) - find_package(OpenMP REQUIRED) -else() - message(STATUS "Disabling OpenMP") -endif() - if(ENABLE_ALL_WARNINGS) foreach(target objtreelite objtreelite_runtime objtreelite_runtime) target_compile_options(${target} PRIVATE -Wall -Wextra) @@ -40,10 +29,6 @@ foreach(lib objtreelite objtreelite_runtime objtreelite_common) else() target_compile_options(${lib} PRIVATE -funroll-loops) endif() - if(USE_OPENMP) - target_link_libraries(${lib} PUBLIC OpenMP::OpenMP_CXX) - target_compile_definitions(${lib} PRIVATE -DTREELITE_OPENMP_SUPPORT) - endif() if(TEST_COVERAGE) if(MSVC) message(FATAL_ERROR "Test coverage not available on Windows") @@ -112,7 +97,6 @@ target_sources(objtreelite ${PROJECT_SOURCE_DIR}/include/treelite/frontend.h ${PROJECT_SOURCE_DIR}/include/treelite/frontend_impl.h ${PROJECT_SOURCE_DIR}/include/treelite/gtil.h - ${PROJECT_SOURCE_DIR}/include/treelite/omp.h ${PROJECT_SOURCE_DIR}/include/treelite/optional.h ${PROJECT_SOURCE_DIR}/include/treelite/thread_local.h ${PROJECT_SOURCE_DIR}/include/treelite/tree.h diff --git a/src/annotator.cc b/src/annotator.cc index 905f7a12..5978b610 100644 --- a/src/annotator.cc +++ b/src/annotator.cc @@ -8,7 +8,6 @@ #include #include #include -#include #include #include #include From 0a198a46d96c54b58c689c01218796f25b6c9e06 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Thu, 15 Jul 2021 18:18:27 -0700 Subject: [PATCH 02/12] Implement parallel for primitive --- src/CMakeLists.txt | 1 + src/annotator.cc | 32 ++++++------- src/threading_utils/parallel_for.h | 74 ++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 19 deletions(-) create mode 100644 src/threading_utils/parallel_for.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 51a00e65..b60f20d2 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -83,6 +83,7 @@ target_sources(objtreelite gtil/predict.cc gtil/pred_transform.h gtil/pred_transform.cc + threading_utils/parallel_for.h annotator.cc filesystem.cc optable.cc diff --git a/src/annotator.cc b/src/annotator.cc index 5978b610..513c0c0f 100644 --- a/src/annotator.cc +++ b/src/annotator.cc @@ -13,7 +13,9 @@ #include #include #include +#include #include +#include "threading_utils/parallel_for.h" namespace { @@ -72,18 +74,14 @@ inline void ComputeBranchLoopImpl( std::vector> inst(nthread * dmat->num_col, {-1}); const size_t ntree = model.trees.size(); TREELITE_CHECK_LE(rbegin, rend); - TREELITE_CHECK_LT(static_cast(rend), std::numeric_limits::max()); const size_t num_col = dmat->num_col; const ElementType missing_value = dmat->missing_value; const bool nan_missing = treelite::math::CheckNAN(missing_value); - const auto rbegin_i = static_cast(rbegin); - const auto rend_i = static_cast(rend); - #pragma omp parallel for schedule(static) num_threads(nthread) - for (int64_t rid = rbegin_i; rid < rend_i; ++rid) { - const int tid = omp_get_thread_num(); + treelite::threading_utils::ParallelFor(rbegin, rend, nthread, + [&](std::size_t rid, std::size_t thread_id) { const ElementType* row = &dmat->data[rid * num_col]; - const size_t off = dmat->num_col * tid; - const size_t off2 = count_row_ptr[ntree] * tid; + const size_t off = dmat->num_col * thread_id; + const size_t off2 = count_row_ptr[ntree] * thread_id; for (size_t j = 0; j < num_col; ++j) { if (treelite::math::CheckNAN(row[j])) { TREELITE_CHECK(nan_missing) @@ -98,7 +96,7 @@ inline void ComputeBranchLoopImpl( for (size_t j = 0; j < num_col; ++j) { inst[off + j].missing = -1; } - } + }); } template @@ -109,14 +107,10 @@ inline void ComputeBranchLoopImpl( std::vector> inst(nthread * dmat->num_col, {-1}); const size_t ntree = model.trees.size(); TREELITE_CHECK_LE(rbegin, rend); - TREELITE_CHECK_LT(static_cast(rend), std::numeric_limits::max()); - const auto rbegin_i = static_cast(rbegin); - const auto rend_i = static_cast(rend); - #pragma omp parallel for schedule(static) num_threads(nthread) - for (int64_t rid = rbegin_i; rid < rend_i; ++rid) { - const int tid = omp_get_thread_num(); - const size_t off = dmat->num_col * tid; - const size_t off2 = count_row_ptr[ntree] * tid; + treelite::threading_utils::ParallelFor(rbegin, rend, nthread, + [&](std::size_t rid, std::size_t thread_id) { + const size_t off = dmat->num_col * thread_id; + const size_t off2 = count_row_ptr[ntree] * thread_id; const size_t ibegin = dmat->row_ptr[rid]; const size_t iend = dmat->row_ptr[rid + 1]; for (size_t i = ibegin; i < iend; ++i) { @@ -128,7 +122,7 @@ inline void ComputeBranchLoopImpl( for (size_t i = ibegin; i < iend; ++i) { inst[off + dmat->col_ind[i]].missing = -1; } - } + }); } template @@ -198,7 +192,7 @@ AnnotateImpl( count_row_ptr = {0}; const size_t ntree = model.trees.size(); - const int max_thread = omp_get_max_threads(); + const int max_thread = static_cast(std::thread::hardware_concurrency()); nthread = (nthread == 0) ? max_thread : std::min(nthread, max_thread); for (const treelite::Tree& tree : model.trees) { count_row_ptr.push_back(count_row_ptr.back() + tree.num_nodes); diff --git a/src/threading_utils/parallel_for.h b/src/threading_utils/parallel_for.h new file mode 100644 index 00000000..7bde7ac4 --- /dev/null +++ b/src/threading_utils/parallel_for.h @@ -0,0 +1,74 @@ +// +// Created by phcho on 7/15/21. +// + +#ifndef TREELITE_THREADING_UTILS_PARALLEL_FOR_H_ +#define TREELITE_THREADING_UTILS_PARALLEL_FOR_H_ + +#include +#include +#include + +namespace { + +template +std::vector ComputeWorkRange(IndexType begin, IndexType end, std::size_t nthread) { + IndexType num_elem = end - begin; + const IndexType portion = num_elem / nthread + !!(num_elem % nthread); + // integer division, rounded-up + + std::vector work_range(nthread + 1); + work_range[0] = begin; + std::size_t acc = begin; + for (std::size_t i = 0; i < nthread; ++i) { + acc += portion; + work_range[i + 1] = std::min(acc, end); + } + TREELITE_CHECK_EQ(work_range[nthread], end); + + return work_range; +} + +} // anonymous namespace + +namespace treelite { +namespace threading_utils { + +template +void ParallelFor(IndexType begin, IndexType end, std::size_t nthread, FuncType func) { + TREELITE_CHECK_GE(end, begin); + if (begin == end) { + return; + } + /* Divide the rnage [begin, end) equally among the threads. + * The i-th thread gets the range [work_range[i], work_range[i+1]). */ + std::vector work_range = ComputeWorkRange(begin, end, nthread); + + // Launch (nthread - 1) threads, as the main thread should also perform work. + std::vector threads; + for (std::size_t thread_id = 1; thread_id < nthread; ++thread_id) { + threads.emplace_back([&work_range, &func, thread_id]() { + const IndexType begin_ = work_range[thread_id]; + const IndexType end_ = work_range[thread_id + 1]; + for (IndexType i = begin_; i < end_; ++i) { + func(i, thread_id); + } + }); + } + { + const IndexType begin_ = work_range[0]; + const IndexType end_ = work_range[1]; + for (IndexType i = begin_; i < end_; ++i) { + func(i, 0); + } + } + // Join threads + for (std::thread& thread : threads) { + thread.join(); + } +} + +} // namespace threading_utils +} // namespace treelite + +#endif // TREELITE_THREADING_UTILS_PARALLEL_FOR_H_ From d428a432a84040f2447f6a8986928d1f963c4699 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Thu, 15 Jul 2021 19:08:28 -0700 Subject: [PATCH 03/12] Add a test --- src/threading_utils/parallel_for.h | 50 ++++++++++++++------------- tests/cpp/CMakeLists.txt | 3 +- tests/cpp/test_threading_utils.cc | 55 ++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 24 deletions(-) create mode 100644 tests/cpp/test_threading_utils.cc diff --git a/src/threading_utils/parallel_for.h b/src/threading_utils/parallel_for.h index 7bde7ac4..5831cf03 100644 --- a/src/threading_utils/parallel_for.h +++ b/src/threading_utils/parallel_for.h @@ -8,35 +8,17 @@ #include #include #include - -namespace { - -template -std::vector ComputeWorkRange(IndexType begin, IndexType end, std::size_t nthread) { - IndexType num_elem = end - begin; - const IndexType portion = num_elem / nthread + !!(num_elem % nthread); - // integer division, rounded-up - - std::vector work_range(nthread + 1); - work_range[0] = begin; - std::size_t acc = begin; - for (std::size_t i = 0; i < nthread; ++i) { - acc += portion; - work_range[i + 1] = std::min(acc, end); - } - TREELITE_CHECK_EQ(work_range[nthread], end); - - return work_range; -} - -} // anonymous namespace +#include namespace treelite { namespace threading_utils { +template +std::vector ComputeWorkRange(IndexType begin, IndexType end, std::size_t nthread); + template void ParallelFor(IndexType begin, IndexType end, std::size_t nthread, FuncType func) { - TREELITE_CHECK_GE(end, begin); + TREELITE_CHECK_GT(nthread, 0) << "nthread must be positive"; if (begin == end) { return; } @@ -68,6 +50,28 @@ void ParallelFor(IndexType begin, IndexType end, std::size_t nthread, FuncType f } } +template +std::vector ComputeWorkRange(IndexType begin, IndexType end, std::size_t nthread) { + TREELITE_CHECK_GE(end, 0) << "end must be 0 or greater"; + TREELITE_CHECK_GE(begin, 0) << "begin must be 0 or greater"; + TREELITE_CHECK_GE(end, begin) << "end cannot be less than begin"; + TREELITE_CHECK_GT(nthread, 0) << "nthread must be positive"; + IndexType num_elem = end - begin; + const IndexType portion = num_elem / nthread + !!(num_elem % nthread); + // integer division, rounded-up + + std::vector work_range(nthread + 1); + work_range[0] = begin; + IndexType acc = begin; + for (std::size_t i = 0; i < nthread; ++i) { + acc += portion; + work_range[i + 1] = std::min(acc, end); + } + TREELITE_CHECK_EQ(work_range[nthread], end); + + return work_range; +} + } // namespace threading_utils } // namespace treelite diff --git a/tests/cpp/CMakeLists.txt b/tests/cpp/CMakeLists.txt index f331a900..64315e3b 100644 --- a/tests/cpp/CMakeLists.txt +++ b/tests/cpp/CMakeLists.txt @@ -26,10 +26,11 @@ target_sources(treelite_cpp_test test_serializer.cc test_frontend.cc test_compiler_param.cc + test_threading_utils.cc ) target_include_directories(treelite_cpp_test - PRIVATE ../../src/frontend + PRIVATE ../../src/frontend ../../src/ ) msvc_use_static_runtime() diff --git a/tests/cpp/test_threading_utils.cc b/tests/cpp/test_threading_utils.cc new file mode 100644 index 00000000..147e5b28 --- /dev/null +++ b/tests/cpp/test_threading_utils.cc @@ -0,0 +1,55 @@ +/*! + * Copyright (c) 2021 by Contributors + * \file test_threading_utils.cc + * \author Hyunsu Cho + * \brief C++ tests for threading utilities + */ +#include +#include +#include +#include +#include +#include +#include +#include "threading_utils/parallel_for.h" + +using namespace testing; + +namespace treelite { +namespace threading_utils { + +TEST(ThreadingUtils, ComputeWorkRange) { + std::random_device rd; + std::mt19937 rng(rd()); + constexpr int64_t kHigh = 10000; + std::uniform_int_distribution dist(0, kHigh); + std::uniform_int_distribution dist2(1, 100); + + /* Test error handling */ + EXPECT_THAT([&]() { ComputeWorkRange(0, 100, 0); }, + ThrowsMessage(HasSubstr("nthread must be positive"))); + EXPECT_THAT([&]() { ComputeWorkRange(-100, 100, 3); }, + ThrowsMessage(HasSubstr("begin must be 0 or greater"))); + EXPECT_THAT([&]() { ComputeWorkRange(-200, -100, 3); }, + ThrowsMessage(HasSubstr("end must be 0 or greater"))); + EXPECT_THAT([&]() { ComputeWorkRange(200, 100, 3); }, + ThrowsMessage(HasSubstr("end cannot be less than begin"))); + + constexpr int kNumTrial = 1000; + for (int i = 0; i < kNumTrial; ++i) { + int64_t begin = dist(rng); + std::size_t nthread = dist2(rng); + std::uniform_int_distribution dist3(begin, kHigh); + int64_t end = dist3(rng); + auto range = ComputeWorkRange(begin, end, nthread); + EXPECT_EQ(range.size(), nthread + 1); + EXPECT_EQ(range[0], begin); + EXPECT_EQ(range[nthread], end); + for (std::size_t i = 0; i < nthread; ++i) { + EXPECT_GE(range[i + 1], range[i]); + } + } +} + +} // namespace threading_utils +} // namespace treelite From cb78523a0768e26a71f0b93ebeb03e1f6d605e78 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Thu, 15 Jul 2021 23:50:09 -0700 Subject: [PATCH 04/12] Add test for ParallelFor --- src/threading_utils/parallel_for.h | 2 + tests/cpp/test_threading_utils.cc | 117 ++++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 11 deletions(-) diff --git a/src/threading_utils/parallel_for.h b/src/threading_utils/parallel_for.h index 5831cf03..07889815 100644 --- a/src/threading_utils/parallel_for.h +++ b/src/threading_utils/parallel_for.h @@ -19,6 +19,8 @@ std::vector ComputeWorkRange(IndexType begin, IndexType end, std::siz template void ParallelFor(IndexType begin, IndexType end, std::size_t nthread, FuncType func) { TREELITE_CHECK_GT(nthread, 0) << "nthread must be positive"; + TREELITE_CHECK_LE(nthread, std::thread::hardware_concurrency()) + << "nthread cannot exceed " << std::thread::hardware_concurrency(); if (begin == end) { return; } diff --git a/tests/cpp/test_threading_utils.cc b/tests/cpp/test_threading_utils.cc index 147e5b28..17480ba2 100644 --- a/tests/cpp/test_threading_utils.cc +++ b/tests/cpp/test_threading_utils.cc @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -15,16 +16,47 @@ using namespace testing; +namespace { + +class RandomGenerator { + public: + RandomGenerator() + : rng_(std::random_device()()), + int_dist_(std::numeric_limits::min(), std::numeric_limits::max()), + real_dist_(0.0, 1.0) {} + + int64_t DrawInteger(int64_t low, int64_t high) { + TREELITE_CHECK_LT(low, high); + int64_t out = int_dist_(rng_); + int64_t rem = out % (high - low); + int64_t ret; + if (rem < 0) { + ret = high + rem; + } else { + ret = low + rem; + } + TREELITE_CHECK_GE(ret, low); + TREELITE_CHECK_LT(ret, high); + return ret; + } + + double DrawReal(double low, double high) { + TREELITE_CHECK_LT(low, high); + return real_dist_(rng_) * (high - low) + low; + } + + private: + std::mt19937 rng_; + std::uniform_int_distribution int_dist_; + std::uniform_real_distribution real_dist_; +}; + +} // namespace anonymous + namespace treelite { namespace threading_utils { TEST(ThreadingUtils, ComputeWorkRange) { - std::random_device rd; - std::mt19937 rng(rd()); - constexpr int64_t kHigh = 10000; - std::uniform_int_distribution dist(0, kHigh); - std::uniform_int_distribution dist2(1, 100); - /* Test error handling */ EXPECT_THAT([&]() { ComputeWorkRange(0, 100, 0); }, ThrowsMessage(HasSubstr("nthread must be positive"))); @@ -35,12 +67,14 @@ TEST(ThreadingUtils, ComputeWorkRange) { EXPECT_THAT([&]() { ComputeWorkRange(200, 100, 3); }, ThrowsMessage(HasSubstr("end cannot be less than begin"))); - constexpr int kNumTrial = 1000; + /* Property-based testing with randomly generated parameters */ + RandomGenerator rng; + + constexpr int kNumTrial = 200; for (int i = 0; i < kNumTrial; ++i) { - int64_t begin = dist(rng); - std::size_t nthread = dist2(rng); - std::uniform_int_distribution dist3(begin, kHigh); - int64_t end = dist3(rng); + int64_t begin = rng.DrawInteger(0, 10000); + std::size_t nthread = static_cast(rng.DrawInteger(1, 100)); + int64_t end = rng.DrawInteger(begin, 10000); auto range = ComputeWorkRange(begin, end, nthread); EXPECT_EQ(range.size(), nthread + 1); EXPECT_EQ(range[0], begin); @@ -49,6 +83,67 @@ TEST(ThreadingUtils, ComputeWorkRange) { EXPECT_GE(range[i + 1], range[i]); } } + // Test the case with begin == end + for (int i = 0; i < 10; ++i) { + int64_t begin = rng.DrawInteger(0, 10000); + int64_t end = begin; + std::size_t nthread = static_cast(rng.DrawInteger(1, 100)); + auto range = ComputeWorkRange(begin, end, nthread); + EXPECT_EQ(range.size(), nthread + 1); + EXPECT_EQ(range[0], begin); + EXPECT_EQ(range[nthread], begin); + for (std::size_t i = 0; i < nthread; ++i) { + EXPECT_EQ(range[i + 1], range[i]); + } + } +} + +TEST(ThreadingUtils, ParallelFor) { + /* Test error handling */ + const int max_thread = std::thread::hardware_concurrency(); + + auto dummy_func = [](int, std::size_t) {}; + EXPECT_THAT([&]() { ParallelFor(0, 100, 0, dummy_func); }, + ThrowsMessage(HasSubstr("nthread must be positive"))); + EXPECT_THAT([&]() { ParallelFor(200, 100, 3, dummy_func); }, + ThrowsMessage(HasSubstr("end cannot be less than begin"))); + EXPECT_THAT([&]() { ParallelFor(-100, 100, 3, dummy_func); }, + ThrowsMessage(HasSubstr("begin must be 0 or greater"))); + EXPECT_THAT([&]() { ParallelFor(-200, -100, 3, dummy_func); }, + ThrowsMessage(HasSubstr("end must be 0 or greater"))); + EXPECT_THAT([&]() { ParallelFor(200, 100, 3, dummy_func); }, + ThrowsMessage(HasSubstr("end cannot be less than begin"))); + EXPECT_THAT([&]() { ParallelFor(10, 20, 3 * max_thread, dummy_func); }, + ThrowsMessage(HasSubstr( + std::string("nthread cannot exceed ") + std::to_string(max_thread)))); + + /* Property-based testing with randomly generated parameters */ + constexpr int kVectorLength = 10000; + RandomGenerator rng; + std::vector a(kVectorLength); + std::vector b(kVectorLength); + std::generate_n(a.begin(), kVectorLength, [&rng]() { return rng.DrawReal(-1.0, 1.0); }); + std::generate_n(b.begin(), kVectorLength, [&rng]() { return rng.DrawReal(-10.0, 10.0); }); + + constexpr int kNumTrial = 200; + for (int i = 0; i < kNumTrial; ++i) { + std::vector c(kVectorLength); + // Fill c with dummy values + std::generate_n(c.begin(), kVectorLength, [&rng]() { return rng.DrawReal(100.0, 200.0); }); + + // Compute c := a + b on range [begin, end) + int64_t begin = rng.DrawInteger(0, kVectorLength); + std::size_t nthread = static_cast(rng.DrawInteger(1, max_thread + 1)); + int64_t end = rng.DrawInteger(begin, kVectorLength); + + ParallelFor(begin, end, nthread, [&a, &b, &c](int64_t i, std::size_t) { + c[i] = a[i] + b[i]; + }); + + for (int64_t i = begin; i < end; ++i) { + EXPECT_FLOAT_EQ(c[i], a[i] + b[i]) << ", at index " << i; + } + } } } // namespace threading_utils From f6259999998f5e9fba2895653ab77f5b94fddad1 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Thu, 15 Jul 2021 23:52:01 -0700 Subject: [PATCH 05/12] Fix formatting --- src/threading_utils/parallel_for.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/threading_utils/parallel_for.h b/src/threading_utils/parallel_for.h index 07889815..6d089bd0 100644 --- a/src/threading_utils/parallel_for.h +++ b/src/threading_utils/parallel_for.h @@ -1,14 +1,17 @@ -// -// Created by phcho on 7/15/21. -// - +/*! +* Copyright (c) 2021 by Contributors +* \file parallel_for.h +* \brief Implemenation of parallel for loop +* \author Hyunsu Cho +*/ #ifndef TREELITE_THREADING_UTILS_PARALLEL_FOR_H_ #define TREELITE_THREADING_UTILS_PARALLEL_FOR_H_ +#include #include +#include #include #include -#include namespace treelite { namespace threading_utils { From 0587614441dd7ea92ce2d189dad59a4005452ff1 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Thu, 15 Jul 2021 23:57:39 -0700 Subject: [PATCH 06/12] Require Threads in CMake build --- cmake/TreeliteConfig.cmake.in | 4 ++++ src/CMakeLists.txt | 3 +++ tests/travis/run_test.sh | 6 +++--- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/cmake/TreeliteConfig.cmake.in b/cmake/TreeliteConfig.cmake.in index b305a2ab..18a875c4 100644 --- a/cmake/TreeliteConfig.cmake.in +++ b/cmake/TreeliteConfig.cmake.in @@ -1,5 +1,9 @@ @PACKAGE_INIT@ +include(CMakeFindDependencyMacro) + +find_dependency(Threads) + if(NOT TARGET treelite::treelite) include(${CMAKE_CURRENT_LIST_DIR}/TreeliteTargets.cmake) endif() diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index b60f20d2..7fb97183 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -11,6 +11,8 @@ endif(UNIX) add_library(objtreelite_common OBJECT) # Component shared by both main package and runtime +find_package(Threads REQUIRED) + if(ENABLE_ALL_WARNINGS) foreach(target objtreelite objtreelite_runtime objtreelite_runtime) target_compile_options(${target} PRIVATE -Wall -Wextra) @@ -22,6 +24,7 @@ foreach(lib objtreelite objtreelite_runtime objtreelite_common) $ $ $/include>) + target_link_libraries(${lib} PUBLIC Threads::Threads) if(MSVC) target_compile_options(${lib} PRIVATE /MP) target_compile_definitions(${lib} PRIVATE -DNOMINMAX) diff --git a/tests/travis/run_test.sh b/tests/travis/run_test.sh index 012e692b..cde1c72e 100755 --- a/tests/travis/run_test.sh +++ b/tests/travis/run_test.sh @@ -15,7 +15,7 @@ then rm -rf build/ mkdir build cd build - cmake .. -DTEST_COVERAGE=ON -DUSE_OPENMP=ON -DBUILD_CPP_TEST=ON -GNinja + cmake .. -DTEST_COVERAGE=ON -DBUILD_CPP_TEST=ON -GNinja ninja cd .. conda install -c conda-forge numpy scipy pandas pytest pytest-cov scikit-learn coverage @@ -42,7 +42,7 @@ then rm -rf build/ mkdir build cd build - cmake .. -DCMAKE_INSTALL_PREFIX="$CONDA_PREFIX" -DCMAKE_INSTALL_LIBDIR="lib" -DUSE_OPENMP=ON -DBUILD_STATIC_LIBS=ON -GNinja + cmake .. -DCMAKE_INSTALL_PREFIX="$CONDA_PREFIX" -DCMAKE_INSTALL_LIBDIR="lib" -DBUILD_STATIC_LIBS=ON -GNinja ninja install # Try compiling a sample application @@ -66,7 +66,7 @@ then rm -rf build/ mkdir build cd build - cmake .. -DUSE_OPENMP=ON -GNinja + cmake .. -GNinja ninja cd .. rm -rfv python/dist python/build From 9be928353c8d86919fb1f2e8e8e97b006f1c4a50 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Fri, 16 Jul 2021 00:27:28 -0700 Subject: [PATCH 07/12] Simplify assertion --- tests/cpp/test_threading_utils.cc | 33 ++++++++++--------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/tests/cpp/test_threading_utils.cc b/tests/cpp/test_threading_utils.cc index 17480ba2..5adf4b3b 100644 --- a/tests/cpp/test_threading_utils.cc +++ b/tests/cpp/test_threading_utils.cc @@ -14,8 +14,6 @@ #include #include "threading_utils/parallel_for.h" -using namespace testing; - namespace { class RandomGenerator { @@ -58,14 +56,10 @@ namespace threading_utils { TEST(ThreadingUtils, ComputeWorkRange) { /* Test error handling */ - EXPECT_THAT([&]() { ComputeWorkRange(0, 100, 0); }, - ThrowsMessage(HasSubstr("nthread must be positive"))); - EXPECT_THAT([&]() { ComputeWorkRange(-100, 100, 3); }, - ThrowsMessage(HasSubstr("begin must be 0 or greater"))); - EXPECT_THAT([&]() { ComputeWorkRange(-200, -100, 3); }, - ThrowsMessage(HasSubstr("end must be 0 or greater"))); - EXPECT_THAT([&]() { ComputeWorkRange(200, 100, 3); }, - ThrowsMessage(HasSubstr("end cannot be less than begin"))); + EXPECT_THROW(ComputeWorkRange(0, 100, 0), treelite::Error); + EXPECT_THROW(ComputeWorkRange(-100, 100, 3), treelite::Error); + EXPECT_THROW(ComputeWorkRange(-200, -100, 3), treelite::Error); + EXPECT_THROW(ComputeWorkRange(200, 100, 3), treelite::Error); /* Property-based testing with randomly generated parameters */ RandomGenerator rng; @@ -103,19 +97,12 @@ TEST(ThreadingUtils, ParallelFor) { const int max_thread = std::thread::hardware_concurrency(); auto dummy_func = [](int, std::size_t) {}; - EXPECT_THAT([&]() { ParallelFor(0, 100, 0, dummy_func); }, - ThrowsMessage(HasSubstr("nthread must be positive"))); - EXPECT_THAT([&]() { ParallelFor(200, 100, 3, dummy_func); }, - ThrowsMessage(HasSubstr("end cannot be less than begin"))); - EXPECT_THAT([&]() { ParallelFor(-100, 100, 3, dummy_func); }, - ThrowsMessage(HasSubstr("begin must be 0 or greater"))); - EXPECT_THAT([&]() { ParallelFor(-200, -100, 3, dummy_func); }, - ThrowsMessage(HasSubstr("end must be 0 or greater"))); - EXPECT_THAT([&]() { ParallelFor(200, 100, 3, dummy_func); }, - ThrowsMessage(HasSubstr("end cannot be less than begin"))); - EXPECT_THAT([&]() { ParallelFor(10, 20, 3 * max_thread, dummy_func); }, - ThrowsMessage(HasSubstr( - std::string("nthread cannot exceed ") + std::to_string(max_thread)))); + EXPECT_THROW(ParallelFor(0, 100, 0, dummy_func), treelite::Error); + EXPECT_THROW(ParallelFor(200, 100, 3, dummy_func), treelite::Error); + EXPECT_THROW(ParallelFor(-100, 100, 3, dummy_func), treelite::Error); + EXPECT_THROW(ParallelFor(-200, -100, 3, dummy_func), treelite::Error); + EXPECT_THROW(ParallelFor(200, 100, 3, dummy_func), treelite::Error); + EXPECT_THROW(ParallelFor(10, 20, 3 * max_thread, dummy_func), treelite::Error); /* Property-based testing with randomly generated parameters */ constexpr int kVectorLength = 10000; From d469dadf00ee2682b64ddb641a747fdfd9e0647b Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Fri, 16 Jul 2021 00:32:30 -0700 Subject: [PATCH 08/12] Don't run Homebrew commands, to save CI time --- tests/travis/run_test.sh | 8 ++++---- tests/travis/setup.sh | 6 ------ 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/travis/run_test.sh b/tests/travis/run_test.sh index cde1c72e..fbf9a409 100755 --- a/tests/travis/run_test.sh +++ b/tests/travis/run_test.sh @@ -9,6 +9,7 @@ then conda activate python3 conda --version python --version + conda install -c conda-forge numpy scipy pandas pytest pytest-cov scikit-learn coverage ninja lcov cmake # Run coverage test set -x @@ -18,7 +19,6 @@ then cmake .. -DTEST_COVERAGE=ON -DBUILD_CPP_TEST=ON -GNinja ninja cd .. - conda install -c conda-forge numpy scipy pandas pytest pytest-cov scikit-learn coverage python -m pip install --pre xgboost python -m pip install lightgbm codecov ./build/treelite_cpp_test @@ -36,6 +36,7 @@ then conda activate python3 conda --version python --version + conda install -c conda-forge ninja cmake # Install Treelite C++ library into the Conda env set -x @@ -60,6 +61,7 @@ then conda activate python3 conda --version python --version + conda install -c conda-forge numpy scipy pandas pytest scikit-learn coverage ninja cmake # Build binary wheel set -x @@ -88,7 +90,6 @@ then python -m pip install ./runtime/python/dist/treelite_runtime-*-py3-none-${TAG}.whl # Run tests - conda install -c conda-forge numpy scipy pandas pytest scikit-learn coverage python -m pip install --pre xgboost python -m pip install lightgbm python -m pytest -v --fulltrace tests/python @@ -109,7 +110,7 @@ fi if [ ${TASK} == "python_sdist_test" ]; then conda activate python3 python --version - conda install numpy scipy + conda install -c conda-forge numpy scipy pandas pytest scikit-learn coverage cmake ninja # Build source distribution make pippack @@ -119,7 +120,6 @@ if [ ${TASK} == "python_sdist_test" ]; then python -m pip install -v treelite_runtime-*.tar.gz # Run tests - conda install -c conda-forge numpy scipy pandas pytest scikit-learn coverage python -m pip install --pre xgboost python -m pip install lightgbm python -m pytest -v --fulltrace tests/python diff --git a/tests/travis/setup.sh b/tests/travis/setup.sh index a7b0a2e0..d89b05e2 100755 --- a/tests/travis/setup.sh +++ b/tests/travis/setup.sh @@ -3,12 +3,6 @@ set -eo pipefail if [ ${TRAVIS_OS_NAME} == "osx" ]; then - brew update - # Use libomp 11.1.0: https://github.com/dmlc/xgboost/issues/7039 - wget https://raw.githubusercontent.com/Homebrew/homebrew-core/679923b4eb48a8dc7ecc1f05d06063cd79b3fc00/Formula/libomp.rb -O $(find $(brew --repository) -name libomp.rb) - brew install cmake libomp lcov ninja - brew pin libomp - wget -O conda.sh https://repo.anaconda.com/miniconda/Miniconda3-latest-MacOSX-x86_64.sh else wget -O conda.sh https://repo.anaconda.com/miniconda/Miniconda3-latest-Linux-x86_64.sh From 4d1dd862acfd5f53f29c003cb163130f18b5c209 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Fri, 16 Jul 2021 00:45:07 -0700 Subject: [PATCH 09/12] Install XGBoost and LightGBM without OpenMP --- tests/travis/run_test.sh | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/travis/run_test.sh b/tests/travis/run_test.sh index fbf9a409..ae8f0ae6 100755 --- a/tests/travis/run_test.sh +++ b/tests/travis/run_test.sh @@ -19,8 +19,10 @@ then cmake .. -DTEST_COVERAGE=ON -DBUILD_CPP_TEST=ON -GNinja ninja cd .. - python -m pip install --pre xgboost - python -m pip install lightgbm codecov + # Install XGBoost and LightGBM without OpenMP + python -m pip install --pre xgboost --no-binary :all: + python -m pip install lightgbm --no-binary :all: + python -m pip install codecov ./build/treelite_cpp_test PYTHONPATH=./python:./runtime/python python -m pytest --cov=treelite --cov=treelite_runtime -v --fulltrace tests/python lcov --directory . --capture --output-file coverage.info @@ -90,8 +92,9 @@ then python -m pip install ./runtime/python/dist/treelite_runtime-*-py3-none-${TAG}.whl # Run tests - python -m pip install --pre xgboost - python -m pip install lightgbm + # Install XGBoost and LightGBM without OpenMP + python -m pip install --pre xgboost --no-binary :all: + python -m pip install lightgbm --no-binary :all: python -m pytest -v --fulltrace tests/python # Deploy binary wheel to S3 @@ -120,8 +123,9 @@ if [ ${TASK} == "python_sdist_test" ]; then python -m pip install -v treelite_runtime-*.tar.gz # Run tests - python -m pip install --pre xgboost - python -m pip install lightgbm + # Install XGBoost and LightGBM without OpenMP + python -m pip install --pre xgboost --no-binary :all: + python -m pip install lightgbm --no-binary :all: python -m pytest -v --fulltrace tests/python # Deploy source wheel to S3 From 7756deb2a081d888ad67b3f1eb1575232dfae990 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Fri, 16 Jul 2021 02:00:00 -0700 Subject: [PATCH 10/12] Address reviewer's feedback --- src/threading_utils/parallel_for.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/threading_utils/parallel_for.h b/src/threading_utils/parallel_for.h index 6d089bd0..c966b779 100644 --- a/src/threading_utils/parallel_for.h +++ b/src/threading_utils/parallel_for.h @@ -8,6 +8,7 @@ #define TREELITE_THREADING_UTILS_PARALLEL_FOR_H_ #include +#include #include #include #include @@ -27,20 +28,20 @@ void ParallelFor(IndexType begin, IndexType end, std::size_t nthread, FuncType f if (begin == end) { return; } - /* Divide the rnage [begin, end) equally among the threads. + /* Divide the range [begin, end) equally among the threads. * The i-th thread gets the range [work_range[i], work_range[i+1]). */ std::vector work_range = ComputeWorkRange(begin, end, nthread); // Launch (nthread - 1) threads, as the main thread should also perform work. - std::vector threads; + std::vector> async_tasks; for (std::size_t thread_id = 1; thread_id < nthread; ++thread_id) { - threads.emplace_back([&work_range, &func, thread_id]() { + async_tasks.push_back(std::async(std::launch::async, [&work_range, &func, thread_id]() { const IndexType begin_ = work_range[thread_id]; const IndexType end_ = work_range[thread_id + 1]; for (IndexType i = begin_; i < end_; ++i) { func(i, thread_id); } - }); + })); } { const IndexType begin_ = work_range[0]; @@ -50,8 +51,8 @@ void ParallelFor(IndexType begin, IndexType end, std::size_t nthread, FuncType f } } // Join threads - for (std::thread& thread : threads) { - thread.join(); + for (auto& task : async_tasks) { + task.get(); } } From 49816272916992b679f46b4e5f5e04976fe4b214 Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Fri, 16 Jul 2021 02:01:50 -0700 Subject: [PATCH 11/12] Install Gtest in Conda to speed up build --- tests/travis/run_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/travis/run_test.sh b/tests/travis/run_test.sh index ae8f0ae6..f18e9095 100755 --- a/tests/travis/run_test.sh +++ b/tests/travis/run_test.sh @@ -9,7 +9,7 @@ then conda activate python3 conda --version python --version - conda install -c conda-forge numpy scipy pandas pytest pytest-cov scikit-learn coverage ninja lcov cmake + conda install -c conda-forge numpy scipy pandas pytest pytest-cov scikit-learn coverage ninja lcov cmake gtest gmock # Run coverage test set -x From f206ffeb7e0fccccfe2124e3c596523261193c5d Mon Sep 17 00:00:00 2001 From: Hyunsu Cho Date: Fri, 16 Jul 2021 02:09:50 -0700 Subject: [PATCH 12/12] Revert "Install Gtest in Conda to speed up build" This reverts commit 49816272916992b679f46b4e5f5e04976fe4b214. --- tests/travis/run_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/travis/run_test.sh b/tests/travis/run_test.sh index f18e9095..ae8f0ae6 100755 --- a/tests/travis/run_test.sh +++ b/tests/travis/run_test.sh @@ -9,7 +9,7 @@ then conda activate python3 conda --version python --version - conda install -c conda-forge numpy scipy pandas pytest pytest-cov scikit-learn coverage ninja lcov cmake gtest gmock + conda install -c conda-forge numpy scipy pandas pytest pytest-cov scikit-learn coverage ninja lcov cmake # Run coverage test set -x