-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added a formula evaluator to replace TFormula calls #11516
Added a formula evaluator to replace TFormula calls #11516
Conversation
Created a reco::FormulaEvaluator class which can parse a subset of the TFormula expressions. The parsing and function evaluation are done using stateless classes so are thread safe and extremely thread efficient. The formula parser is shared by all reco::FormulaEvaluators to save memory and since the parser is stateless it can efficiently be called by multiple threads simultaneously.
A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_6_X. Added a formula evaluator to replace TFormula calls It involves the following packages: CommonTools/Utils @cmsbuild, @cvuosalo, @vadler, @monttj, @slava77 can you please review it and eventually sign? Thanks. |
please test |
The tests are being triggered in jenkins. |
My intentions is to try this in |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
{ | ||
|
||
public: | ||
enum class Precidence { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it Precedence or my poor English?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because I'm a native speaker doesn't mean my English is any good :)
With this change added on top of the changes to ROOT and the pull request #11469 I am getting significantly improved performance on an Tier 0 like job when using 8 threads. I'm working on getting the precise measurements now but the short job I ran for VTune showed no major stalls or slowdowns due to unwanted synchronization. |
@Dr15Jones I think I will start anew from something like this (or a hybrid of what already exists) for the StringCutObjectParser. |
@Dr15Jones Wouldn't it be possible to cover N input args (even zero) using variadic templates? Can poke at that if needed. |
From a threading stand point the fix I did to cache the function pointer created by cling pretty much solved the StringCutObjectParser efficiency problem. |
The reason I could get away with doing a hand parser and not use boost spirit is everything was a double and the syntax was much simpler. |
@Dr15Jones Hmm, should I continue with that project then? boost::spirit isn't the memory hog, it's really clang, which I think we will keep around any way due to simply reading our root data. |
@Dr15Jones Probably a better use of my time is completely getting rid of TMVA (where possible) for BDTs. |
Removing the use of TMVA is probably better since the cut parser inefficiency seems to be under control. |
Added a formula evaluator to replace TFormula calls
Created a reco::FormulaEvaluator class which can parse a subset of
the TFormula expressions. The parsing and function evaluation are
done using stateless classes so are thread safe and extremely thread
efficient. The formula parser is shared by all reco::FormulaEvaluators
to save memory and since the parser is stateless it can efficiently
be called by multiple threads simultaneously.