[RF] Make RooBatchCompute thread safe #14877
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guitargeek
force-pushed
the
batchcompute_no_vectors
branch
from
19906a9
to
09b3913
Compare
guitargeek
force-pushed
the
batchcompute_no_vectors
branch
from
09b3913
to
a99fd34
Compare
Test Results 12 files 12 suites 2d 8h 12m 42s ⏱️ Results for commit 2abf7e5. ♻️ This comment has been updated with latest results. |
This can be quite a big overhead if the dataset is small.
guitargeek
force-pushed
the
batchcompute_no_vectors
branch
from
a99fd34
to
32dc1a5
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2004/python3. Errors:
|
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/soversion. Errors:
|
|
Build failed on windows10/default. Errors:
|
guitargeek
force-pushed
the
batchcompute_no_vectors
branch
from
32dc1a5
to
174c302
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Errors:
|
|
Build failed on ROOT-ubuntu2004/python3. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/soversion. Errors:
|
|
Build failed on windows10/default. Errors:
|
guitargeek
force-pushed
the
batchcompute_no_vectors
branch
from
174c302
to
2abf7e5
Compare
|
Starting build on |
In the RooBatchCompute CPU library, all scalar inputs have to be copied n times into a buffer that is as long as the SIMD registers, to allow for vectorization in all cases. To avoid frequent memory allocations, this buffer was made a `static` variable in the original implementation of the batchcompute library, which of course made it non-threadsafe. This is now hitting us, because RooFit needs to be able to do multiple fits concurrently. This is a requirement for CMSSW, and a blocker for ROOT master adoption in CMSSW since the new CPU backend is the default: cms-sw/cmsdist#9034 This commit fixes the concurrency problem by doing the buffering in the DataMaps that are used in the `RooFit::Evaluator`. Like this, multiple computation graphs can be concurrently evaluated. It was tested with the ATLAS benchmarks in `rootbench` that the fitting performance remains the same.
The Batches classes in RooBatchCompute have only public data members anyway. It's not necessary to have extra member functions for changing them.
|
Build failed on windows10/default. Errors:
And 71 more |
|
The failures on Windows Jenkins are the occasional out-of-heap-space of the compiler. It has nothing to do with this PR, and the new CI is green for Windows 32 bit and 64 bit. |
lmoneta
approved these changes
Mar 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the RooBatchCompute CPU library, all scalar inputs have to be copied
n times into a buffer that is as long as the SIMD registers, to allow
for vectorization in all cases.
To avoid frequent memory allocations, this buffer was made a
staticvariable in the original implementation of the batchcompute library,
which of course made it non-threadsafe.
This is now hitting us, because RooFit needs to be able to do multiple
fits concurrently. This is a requirement for CMSSW, and a blocker for
ROOT master adoption in CMSSW since the new CPU backend is the default:
cms-sw/cmsdist#9034
This commit fixes the concurrency problem by doing the buffering in the
DataMaps that are used in the
RooFit::Evaluator. Like this, multiplecomputation graphs can be concurrently evaluated.
It was tested with the ATLAS benchmarks in
rootbenchthat the fittingperformance remains the same.
This PR also makes some code style and memory management improvements documented in the commit messages.
It also updates the documentation to make clear the new CPU backend is now the default, which was not done in this PR:
#14742