Add TBB-based counter for global read-write lock by bendavid · Pull Request #7260 · root-project/root

bendavid · 2021-02-19T19:59:48Z

Adds TBB-based counter types for global read-write locks and use one by default when TBB is available.

This gives a 7% speedup in a medium sized test filling histograms from CMS NanoAOD with RDF and 256 threads.

This implements the last part of the improvements originally discussed in #6919

phsft-bot · 2021-02-19T19:59:50Z

Can one of the admins verify this patch?

oshadura · 2021-03-04T09:32:46Z

@phsft-bot build!

phsft-bot · 2021-03-04T09:32:53Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-03-04T09:56:55Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on null:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-03-04T09:59:16Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-03-04T10:11:36Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-03-04T10:33:58Z

Build failed on mac1014/python3.
Running on macphsft17.dyndns.cern.ch:/build/jenkins/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-03-04T10:39:07Z

Build failed on ROOT-fedora31/noimt.
Running on root-fedora-31-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-03-04T11:04:26Z

Build failed on mac11.0/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-03-04T11:10:00Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

oshadura · 2021-03-04T16:26:14Z

@bendavid

2:09:55    In file included from input_line_7:20:
12:09:55    In file included from C:/build/workspace/root-pullrequests-build/build/include\ROOT/TReentrantRWLock.hxx:26:
12:09:55    In file included from C:/build/workspace/root-pullrequests-build/build/include\tbb/enumerable_thread_specific.h:23:
12:09:55    In file included from C:/build/workspace/root-pullrequests-build/build/include\tbb/atomic.h:42:
12:09:55    In file included from C:/build/workspace/root-pullrequests-build/build/include\tbb/tbb_machine.h:113:
12:09:55    In file included from C:/build/workspace/root-pullrequests-build/build/include\tbb/tbb_stddef.h:119:
12:09:55  C:/build/workspace/root-pullrequests-build/build/include\tbb/internal/_tbb_windef.h(28,2): error GAAAB8493: TBB requires linkage with multithreaded C/C++ runtime library.        Choose multithreaded DLL runtime in project settings, or use /MD[d] compiler switch. [C:\build\workspace\root-pullrequests-build\build\core\thread\G__Thread.vcxproj]
12:09:55    #error TBB requires linkage with multithreaded C/C++ runtime library. \
12:09:55     ^
12:09:56  CUSTOMBUILD : error : C:/build/workspace/root-pullrequests-build/build/bin/rootcling_stage1.exe: compilation failure (C:/build/workspace/root-pullrequests-build/build/bin/libThreada1d22a0ca9_dictUmbrella.h) [C:\build\workspace\root-pullrequests-build\build\core\thread\G__Thread.vcxproj]
12:09:56    G__multimap2Dict.vcxproj -> C:\build\workspace\root-pullrequests-build\build\core\clingutils\G__multimap2Dict.dir\Release\G__multimap2Dict.lib
12:09:56    G__unordered_mapDict.vcxproj -> C:\build\workspace\root-pullrequests-build\build\core\clingutils\G__unordered_mapDict.dir\Release\G__unordered_mapDict.lib
12:09:57    G__forward_listDict.vcxproj -> C:\build\workspace\root-pullrequests-build\build\core\clingutils\G__forward_listDict.dir\Release\G__forward_listDict.lib
12:09:57  Command exited with the value: 1
12:09:57  MakeCommand:"C:\Program Files\CMake\bin\cmake.exe" --build . --config "Release" -- /maxcpucount:16
12:09:57  Error(s) when building project
12:09:57     2 Compiler errors

bendavid · 2021-03-04T16:29:48Z

Currently the usage of TBB in core/thread is triggered by the availability of tbb in the build (standalone or builtin). Should the criteria be something different/more selective? (or is there some other switch which needs to be triggered in the windows build for this case?)

bendavid · 2021-03-14T17:00:08Z

Alternatively this could be steered by whether the build is done with implicit multithreading enabled, rather than directly checking the availability of TBB (since this seems to not be sufficient in the windows case) Thoughts?

pcanal · 2021-03-15T23:29:27Z

I suspect that instead we should still rely on detecting the availability of TBB but disable this feature on Windows, at least for now. (I.e. the condition is "has a TBB and not on Windows").

bendavid · 2021-03-16T00:04:10Z

Alright, done.

pcanal · 2021-03-16T18:08:17Z

core/thread/test/CMakeLists.txt

+   ROOT_ADD_UNITTEST_DIR(Core Thread Hist ${TBB_LIBRARIES})

-ROOT_ADD_GTEST(testTThreadedObject testTThreadedObject.cxx LIBRARIES Hist)
+   ROOT_ADD_GTEST(testTThreadedObject testTThreadedObject.cxx LIBRARIES Hist ${TBB_LIBRARIES})


Why is this needed? Isn't tbb already linked through libThread (and/or libImt)?

Yes I think you're right, will remove it.

pcanal · 2021-03-16T18:12:24Z

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

phsft-bot · 2021-03-16T18:12:38Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

phsft-bot · 2021-03-16T19:13:47Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.runtutorials.tutorial_dataframe_df010_trivialDataSource

phsft-bot · 2021-03-17T01:29:43Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.runtutorials.tutorial_v7_global_temperatures.cxx

phsft-bot · 2021-03-17T03:28:47Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

projectroot.runtutorials.tutorial_v7_global_temperatures.cxx

bendavid · 2021-03-18T20:06:51Z

Updated to remove the unnecessary bits from the CMakeLists for the tests.

pcanal · 2021-03-22T23:22:14Z

@bendavid I think this is good to go except for adding some trace in the repository (either commit or, probably better, in the release notes) of the performance improvement.

bendavid · 2021-03-24T14:43:40Z

Thanks, I propose to add the relevant information to the release notes in a separate PR (mainly to avoid an otherwise unnecessary rebase before merging)

pcanal · 2021-03-24T18:18:08Z

Fair enough :)

pcanal · 2021-03-24T18:21:15Z

In the subsequent PR, consider also applying the formatting recommendation at: https://travis-ci.org/github/root-project/root/jobs/763450064

This covers the changes in root-project#6857 root-project#7105 root-project#7260

This covers the changes in #6857 #7105 #7260

This covers the changes in root-project#6857 root-project#7105 root-project#7260

This covers the changes in #6857 #7105 #7260

This covers the changes in root-project#6857 root-project#7105 root-project#7260

@peremato

A long time ago, ROOT had a `tbb` build option, but it was removed in the ROOT 6.08 development cycle in dd899a5 from 2015, by @peremato. However, some build configurations still use it, like CMSSW and Conda: * https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_15_1_X/master/root.spec#L78 * https://github.com/conda-forge/root-feedstock/blob/main/recipe/build_root.sh#L342 That would not be a problem per se, but then, in 2021, @bendavid introduced a new optimization that is enabled only with the long-time removed `tbb` flag (or `builtin_tbb`), in 6c8b77d: * root-project#7260 So we ended up with a secret `tbb=ON` configuration that our CI doesn't test, and a CMake configuration option that has a pretty fundamental effect but is not documented. In the discussions in root-project#19798 with @bendavid, we concluded that it would not be a problem to enable the optimized locks with TBB always when TBB is available during the ROOT build. See: root-project#19798 (comment) Closes root-project#19798.

@peremato

A long time ago, ROOT had a `tbb` build option, but it was removed in the ROOT 6.08 development cycle in dd899a5 from 2015, by @peremato. However, some build configurations still use it, like CMSSW and Conda: * https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_15_1_X/master/root.spec#L78 * https://github.com/conda-forge/root-feedstock/blob/main/recipe/build_root.sh#L342 That would not be a problem per se, but then, in 2021, @bendavid introduced a new optimization that is enabled only with the long-time removed `tbb` flag (or `builtin_tbb`), in 6c8b77d: * #7260 So we ended up with a secret `tbb=ON` configuration that our CI doesn't test, and a CMake configuration option that has a pretty fundamental effect but is not documented. In the discussions in #19798 with @bendavid, we concluded that it would not be a problem to enable the optimized locks with TBB always when TBB is available during the ROOT build. See: #19798 (comment) Closes #19798.

bendavid added 3 commits February 19, 2021 20:39

fix missing/duplicated test

a4502b7

add flags for optional TBB dependencies

038b233

add TBB based RWMutex classes and use by default when TBB is available

48f26f6

bendavid requested review from Axel-Naumann, oshadura and pcanal as code owners February 19, 2021 19:59

etejedor assigned pcanal Feb 22, 2021

pcanal reviewed Mar 16, 2021

View reviewed changes

pcanal mentioned this pull request Mar 16, 2021

Further improvements to reduce lock contention #6919

Closed

add TBB based RWMutex types to tests, plus a few additional tests

4fe7994

disable TBB use on Windows

da2cf1b

bendavid force-pushed the lockimprovementstbb branch from bc19562 to da2cf1b Compare March 18, 2021 20:05

pcanal approved these changes Mar 24, 2021

View reviewed changes

pcanal merged commit 9e6f2b9 into root-project:master Mar 24, 2021

bendavid added a commit to bendavid/root that referenced this pull request Mar 24, 2021

[relnotes] Add lock contention improvements

ab9da43

This covers the changes in root-project#6857 root-project#7105 root-project#7260

bendavid mentioned this pull request Mar 24, 2021

Code formatting and release notes for recent lock contention improvements #7680

Merged

eguiraud pushed a commit that referenced this pull request Mar 25, 2021

[relnotes] Add lock contention improvements

b6de592

This covers the changes in #6857 #7105 #7260

eguiraud pushed a commit to eguiraud/root that referenced this pull request Mar 25, 2021

[relnotes] Add lock contention improvements

39a4631

This covers the changes in root-project#6857 root-project#7105 root-project#7260

eguiraud pushed a commit that referenced this pull request Mar 25, 2021

[relnotes] Add lock contention improvements

78cb977

This covers the changes in #6857 #7105 #7260

pcanal mentioned this pull request Mar 25, 2021

[v624][core] Add TBB-based counter for global read-write lock #7703

Merged

nicknagi pushed a commit to nicknagi/root that referenced this pull request Mar 30, 2021

[relnotes] Add lock contention improvements

372a985

This covers the changes in root-project#6857 root-project#7105 root-project#7260

linev mentioned this pull request Apr 28, 2021

TBB exposed in public includes of Thread library #8022

Closed

guitargeek mentioned this pull request Sep 1, 2025

Removed option tbb is still considered to enable secret optimization of ROOT::gCoreMutex #19798

Closed

guitargeek mentioned this pull request Nov 28, 2025

[CMake] Replace logic that checks removed tbb build option #20564

Merged

Conversation

bendavid commented Feb 19, 2021

Uh oh!

phsft-bot commented Feb 19, 2021

Uh oh!

oshadura commented Mar 4, 2021

Uh oh!

phsft-bot commented Mar 4, 2021

Uh oh!

phsft-bot commented Mar 4, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 4, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 4, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 4, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 4, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 4, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 4, 2021

Uh oh!

oshadura commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendavid commented Mar 4, 2021

Uh oh!

bendavid commented Mar 14, 2021

Uh oh!

pcanal commented Mar 15, 2021

Uh oh!

bendavid commented Mar 16, 2021

Uh oh!

pcanal Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

bendavid Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

pcanal commented Mar 16, 2021

Uh oh!

phsft-bot commented Mar 16, 2021

Uh oh!

phsft-bot commented Mar 16, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 17, 2021

Failing tests:

Uh oh!

phsft-bot commented Mar 17, 2021

Failing tests:

Uh oh!

bendavid commented Mar 18, 2021

Uh oh!

pcanal commented Mar 22, 2021

Uh oh!

bendavid commented Mar 24, 2021

Uh oh!

pcanal commented Mar 24, 2021

Uh oh!

pcanal commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oshadura commented Mar 4, 2021 •

edited

Loading