[RF] Make multiprocess gradient parallelization work with new vectorizing CPU backend - #23321
Merged
Merged
Conversation
guitargeek
force-pushed
the
parallel-gradient
branch
2 times, most recently
from
September 9, 2026 21:36
ae8afbc to
1e28d69
Compare
Test Results 23 files 23 suites 3d 17h 26m 12s ⏱️ For more details on these failures, see this check. Results for commit d34268c. ♻️ This comment has been updated with latest results. |
guitargeek
force-pushed
the
parallel-gradient
branch
from
September 13, 2026 10:26
1e28d69 to
28528a1
Compare
The evaluator-backed RooUnbinnedL computed the negative log-likelihood from the batch of probabilities with a scalar std::log loop, costing several times more than one vectorized likelihood pass now that the rest of the evaluation is SIMD. Use the same RooBatchCompute::reduceNLL() reduction as RooNLLVarNew, which also reproduces the RooNaNPacker-based error propagation of the loop (badness packed into the returned NaN). 🤖 Done with the help of AI
The ModularL branch of createNLL never passed the parsed EvalBackend on to the NLLFactory, whose default is the legacy backend. As a result, every parallel fit (fitTo with Parallelize(), which implies ModularL) silently evaluated the deprecated legacy scalar likelihood on the workers, costing about 7x per likelihood evaluation compared to the vectorized CPU backend and making the parallel gradient lose against any serial fit (benchmark: 8-channel unbinned simultaneous fit with 64 correlated constrained systematics, 96 free parameters, 200k events: serial fitTo 9.9s, Parallelize(8) 23.7s before, 4.7s after this and the accompanying gradient-job commits). The bitwise legacy-vs-modular comparisons in testLikelihoodGradientJob now request EvalBackend(Legacy) explicitly on the modular side: they compare against a legacy reference fit, so both likelihoods must use the same arithmetic (previously that happened by virtue of this bug). 🤖 Done with the help of AI
Every gradient calculation in LikelihoodGradientJob started with each worker evaluating the full likelihood once at the central point, only to obtain the scalar function value that NumericalDerivator needs for its step-size tolerances (SetupDifferentiate). Minuit already knows this exact value: the line search that precedes each gradient request stores it in MinimumParameters::Fval(). Hand that value through a new fifth argument of FCNBase::GradientWithPrevResult() (a backward-compatible overload that falls back to the old virtual), broadcast it to the workers along with the rest of the minimizer state, and pre-seed the derivator's existing central-value cache (fVxFValCache) with it, so SetupDifferentiate skips its function call. When the likelihood offsets changed in the same state update, NaN is broadcast instead and the workers evaluate as before, since the known value corresponds to the previous offsets. This is bitwise-transparent: the exact-equality comparisons in testLikelihoodGradientJob pass unchanged with the pre-seeding active. 🤖 Done with the help of AI
guitargeek
force-pushed
the
parallel-gradient
branch
from
September 13, 2026 15:34
28528a1 to
d34268c
Compare
dpiparo
approved these changes
Sep 14, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The parallel gradient was still silently evaluating the likelihood with the RooFit legacy scalar backend on the workers, which heavily penalized it compared to the new default vectorizing CPU backend. This had to be fixed to give the gradient parallelization a chance at proving its worth.
And indeed, it works quite well!
createNLL/FitHelpersnow forward theEvalBackendinto the modular likelihood, so workers use the vectorized CPU backend instead of the legacy scalar one (this was a one-line wiring bug that cost ~7x per evaluation; the legacy-vs-modular gtests passed because of it, and now pinEvalBackend::Legacyexplicitly).RooUnbinnedLreplaces its scalarstd::logreduction loop with the sameRooBatchCompute::reduceNLL/reduceSumused byRooNLLVarNew, with identical NaN-packing error semantics.FCNBase::GradientWithPrevResult), skipping one full likelihood evaluation per worker per gradient call.A toy model with 96 parameters over 8 channels nicely illustrates the performance gains (median of 3 interleaved repeats, 12-core machine, Minuit strategy 0, Migrad only):
fitToParallelize(8), unpatched masterParallelize(1)Parallelize(2)Parallelize(4)Parallelize(8)So for this toy model, you get a solid 2x already with four processes.
This PR therefore brings the gradient parallelization back into the discussion space when deciding how to make RooFit faster.