From 197c1dacd5725db8ab2faf0aa9b39a2655eb4940 Mon Sep 17 00:00:00 2001 From: Ashod Nakashian Date: Tue, 14 Jun 2016 07:19:20 -0400 Subject: tdf#98955 hardware_concurrency not ideal for thread pools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A new static member getPreferredConcurrency added to comphelper::ThreadPool to return a configurable max number of threads. By default the new function returns the hardware_concurrency value provided by std::thread. When MAX_CONCURRENCY envar is defined, the return value is limited to whatever is set there. Three call-sites that used std::thread::hardware_concurrency have been replaced with getPreferredConcurrency. Unittests added to cover the functionality of the new member. Unittests are capped to 4 threads. Reviewed-on: https://gerrit.libreoffice.org/26254 Tested-by: Jenkins Reviewed-by: Ashod Nakashian (cherry picked from commit 60e75fb276778459f6055360646d879b8c615d83) Change-Id: I3332e393a88a5ed436316fa712ed920a4b37f4af Reviewed-on: https://gerrit.libreoffice.org/26394 Tested-by: Jenkins Reviewed-by: Caolán McNamara Tested-by: Caolán McNamara --- .../CppunitTest_comphelper_threadpool_test.mk | 30 ++++++++++++ comphelper/Module_comphelper.mk | 1 + comphelper/qa/unit/threadpooltest.cxx | 55 ++++++++++++++++++++++ comphelper/source/misc/threadpool.cxx | 26 +++++++++- include/comphelper/threadpool.hxx | 6 +++ package/source/zippackage/ZipPackageStream.cxx | 2 +- sc/source/filter/excel/xetable.cxx | 2 +- sc/source/filter/oox/workbookfragment.cxx | 2 +- solenv/gbuild/CppunitTest.mk | 3 ++ 9 files changed, 122 insertions(+), 5 deletions(-) create mode 100644 comphelper/CppunitTest_comphelper_threadpool_test.mk create mode 100644 comphelper/qa/unit/threadpooltest.cxx diff --git a/comphelper/CppunitTest_comphelper_threadpool_test.mk b/comphelper/CppunitTest_comphelper_threadpool_test.mk new file mode 100644 index 000000000000..aa4e0f020a6a --- /dev/null +++ b/comphelper/CppunitTest_comphelper_threadpool_test.mk @@ -0,0 +1,30 @@ +# -*- Mode: makefile-gmake; tab-width: 4; indent-tabs-mode: t -*- +# +# This file is part of the LibreOffice project. +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# + +$(eval $(call gb_CppunitTest_CppunitTest,comphelper_threadpool_test)) + +$(eval $(call gb_CppunitTest_add_exception_objects,comphelper_threadpool_test, \ + comphelper/qa/unit/threadpooltest \ +)) + +$(eval $(call gb_CppunitTest_use_externals,comphelper_threadpool_test,\ + boost_headers \ +)) + +$(eval $(call gb_CppunitTest_use_sdk_api,comphelper_threadpool_test)) + +$(eval $(call gb_CppunitTest_use_libraries,comphelper_threadpool_test, \ + comphelper \ + cppuhelper \ + cppu \ + sal \ + $(gb_UWINAPI) \ +)) + +# vim: set noet sw=4 ts=4: diff --git a/comphelper/Module_comphelper.mk b/comphelper/Module_comphelper.mk index acd50c6827be..f89626cf2c6e 100644 --- a/comphelper/Module_comphelper.mk +++ b/comphelper/Module_comphelper.mk @@ -28,6 +28,7 @@ $(eval $(call gb_Module_add_subsequentcheck_targets,comphelper,\ )) $(eval $(call gb_Module_add_check_targets,comphelper,\ + CppunitTest_comphelper_threadpool_test \ CppunitTest_comphelper_syntaxhighlight_test \ CppunitTest_comphelper_variadictemplates_test \ CppunitTest_comphelper_test \ diff --git a/comphelper/qa/unit/threadpooltest.cxx b/comphelper/qa/unit/threadpooltest.cxx new file mode 100644 index 000000000000..d71a1116897d --- /dev/null +++ b/comphelper/qa/unit/threadpooltest.cxx @@ -0,0 +1,55 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include +#include "cppunit/TestAssert.h" +#include "cppunit/TestFixture.h" +#include "cppunit/extensions/HelperMacros.h" +#include "cppunit/plugin/TestPlugIn.h" + +#include +#include + +class ThreadPoolTest : public CppUnit::TestFixture +{ +public: + void testPreferredConcurrency(); + + CPPUNIT_TEST_SUITE(ThreadPoolTest); + CPPUNIT_TEST(testPreferredConcurrency); + CPPUNIT_TEST_SUITE_END(); +}; + +void ThreadPoolTest::testPreferredConcurrency() { + + // Check default. + auto nThreads = comphelper::ThreadPool::getPreferredConcurrency(); + sal_Int32 nExpected = 4; // UTs are capped to 4. + CPPUNIT_ASSERT_MESSAGE("Expected no more than 4 threads", nExpected >= nThreads); + +#ifndef _WIN32_WINNT + // The result should be cached, so this should change anything. + nThreads = std::thread::hardware_concurrency() * 2; + setenv("MAX_CONCURRENCY", std::to_string(nThreads).c_str(), true); + nThreads = comphelper::ThreadPool::getPreferredConcurrency(); + CPPUNIT_ASSERT_MESSAGE("Expected no more than hardware threads", + nThreads <= (sal_Int32)std::thread::hardware_concurrency()); + + // Revert and check. Again, nothing should change. + unsetenv("MAX_CONCURRENCY"); + nThreads = comphelper::ThreadPool::getPreferredConcurrency(); + CPPUNIT_ASSERT_MESSAGE("Expected no more than 4 threads", nExpected >= nThreads); +#endif +} + +CPPUNIT_TEST_SUITE_REGISTRATION(ThreadPoolTest); + +CPPUNIT_PLUGIN_IMPLEMENT(); + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/comphelper/source/misc/threadpool.cxx b/comphelper/source/misc/threadpool.cxx index 9837ea6acf87..63c733619455 100644 --- a/comphelper/source/misc/threadpool.cxx +++ b/comphelper/source/misc/threadpool.cxx @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -115,8 +116,8 @@ struct ThreadPoolStatic : public rtl::StaticWithInit< std::shared_ptr< ThreadPoo ThreadPoolStatic > { std::shared_ptr< ThreadPool > operator () () { - sal_Int32 nThreads = std::max( std::thread::hardware_concurrency(), 1U ); - return std::shared_ptr< ThreadPool >( new ThreadPool( nThreads ) ); + const sal_Int32 nThreads = ThreadPool::getPreferredConcurrency(); + return std::make_shared< ThreadPool >( nThreads ); }; }; @@ -125,6 +126,27 @@ ThreadPool& ThreadPool::getSharedOptimalPool() return *ThreadPoolStatic::get().get(); } +sal_Int32 ThreadPool::getPreferredConcurrency() +{ + static sal_Int32 ThreadCount = 0; + if (ThreadCount == 0) + { + const sal_Int32 nHardThreads = std::max(std::thread::hardware_concurrency(), 1U); + sal_Int32 nThreads = nHardThreads; + const char *pEnv = getenv("MAX_CONCURRENCY"); + if (pEnv != nullptr) + { + // Override with user/admin preferrence. + nThreads = rtl_str_toInt32(pEnv, 10); + } + + nThreads = std::min(nHardThreads, nThreads); + ThreadCount = std::max(nThreads, 1); + } + + return ThreadCount; +} + void ThreadPool::waitAndCleanupWorkers() { waitUntilEmpty(); diff --git a/include/comphelper/threadpool.hxx b/include/comphelper/threadpool.hxx index cfa471b60802..2f726f0b406f 100644 --- a/include/comphelper/threadpool.hxx +++ b/include/comphelper/threadpool.hxx @@ -36,6 +36,12 @@ public: /// count for the CPU static ThreadPool& getSharedOptimalPool(); + /// returns a configurable max-concurrency + /// limit to avoid spawning an unnecessarily + /// large number of threads on high-core boxes. + /// MAX_CONCURRENCY envar controls the cap. + static sal_Int32 getPreferredConcurrency(); + ThreadPool( sal_Int32 nWorkers ); virtual ~ThreadPool(); diff --git a/package/source/zippackage/ZipPackageStream.cxx b/package/source/zippackage/ZipPackageStream.cxx index baa288dc9a5d..0ff190a69411 100644 --- a/package/source/zippackage/ZipPackageStream.cxx +++ b/package/source/zippackage/ZipPackageStream.cxx @@ -832,7 +832,7 @@ bool ZipPackageStream::saveChild( // cores and allow 4-times the amount for having the queue well filled. The // 2nd pparameter is the time to wait beweeen cleanups in 10th of a second. // Both values may be added to the configuration settings if needed. - static sal_Int32 nAllowedThreads(std::max(std::thread::hardware_concurrency(), 1U) * 4); + static sal_Int32 nAllowedThreads(comphelper::ThreadPool::getPreferredConcurrency() * 4); rZipOut.reduceScheduledThreadsToGivenNumberOrLess(nAllowedThreads, 1); // Start a new thread deflating this zip entry diff --git a/sc/source/filter/excel/xetable.cxx b/sc/source/filter/excel/xetable.cxx index a13395b9baae..647728d92ef4 100644 --- a/sc/source/filter/excel/xetable.cxx +++ b/sc/source/filter/excel/xetable.cxx @@ -2134,7 +2134,7 @@ void XclExpRowBuffer::Finalize( XclExpDefaultRowData& rDefRowData, const ScfUInt // This is staggeringly slow, and each element operates only // on its own data. const size_t nRows = maRowMap.size(); - const size_t nThreads = nRows < 128 ? 1 : std::max(std::thread::hardware_concurrency(), 1U); + const size_t nThreads = nRows < 128 ? 1 : comphelper::ThreadPool::getPreferredConcurrency(); #else const size_t nThreads = 1; // globally disable multi-threading for now. #endif diff --git a/sc/source/filter/oox/workbookfragment.cxx b/sc/source/filter/oox/workbookfragment.cxx index 4bb9522dd21c..1bfbd5cbd2f7 100644 --- a/sc/source/filter/oox/workbookfragment.cxx +++ b/sc/source/filter/oox/workbookfragment.cxx @@ -306,7 +306,7 @@ public: void importSheetFragments( WorkbookFragment& rWorkbookHandler, SheetFragmentVector& rSheets ) { - sal_Int32 nThreads = std::min( rSheets.size(), (size_t) std::max(std::thread::hardware_concurrency(), 1U) ); + sal_Int32 nThreads = std::min( rSheets.size(), (size_t) comphelper::ThreadPool::getPreferredConcurrency() ); Reference< XComponentContext > xContext = comphelper::getProcessComponentContext(); diff --git a/solenv/gbuild/CppunitTest.mk b/solenv/gbuild/CppunitTest.mk index b90fb7fac7e4..b47391e1b970 100644 --- a/solenv/gbuild/CppunitTest.mk +++ b/solenv/gbuild/CppunitTest.mk @@ -19,6 +19,9 @@ # CppunitTest class +# Cap the number of threads unittests use. +export MAX_CONCURRENCY=4 + gb_CppunitTest_UNITTESTFAILED ?= $(GBUILDDIR)/platform/unittest-failed-default.sh gb_CppunitTest_PYTHONDEPS ?= $(call gb_Library_get_target,pyuno_wrapper) $(if $(SYSTEM_PYTHON),,$(call gb_Package_get_target,python3)) -- cgit v1.2.3