[WIP] Speedup retrieval of branch names #18358
Conversation
Test Results 19 files 19 suites 4d 19h 55m 19s ⏱️ For more details on these failures, see this check. Results for commit 07a421d. ♻️ This comment has been updated with latest results. |
vepadulano
force-pushed
the
rdf-speedup-retrieval-branch-names
branch
from
5e990a6 to
07a421d
Compare
| @@ -139,7 +141,16 @@ void ExploreBranch(TTree &t, std::set<std::string> &bNamesReg, ColumnNames_t &bN | |||
| TBranch *subBranch = static_cast<TBranch *>(sb); | |||
| auto subBranchName = std::string(subBranch->GetName()); | |||
| auto fullName = prefix + subBranchName; | |||
|
|
|||
| auto prefixTokens = ROOT::Split(prefix, ".", true /*skipEmpty*/); | |||
There was a problem hiding this comment.
This may be done once outside of the for
| fullName = std::string{}; | ||
| for (const auto &s : prefixTokens) | ||
| fullName += s + "."; | ||
| for (decltype(subBranchNameTokens.size()) i = 1; i < subBranchNameTokens.size(); i++) |
There was a problem hiding this comment.
If I understand this code correctly you don't really need to Split the strings; can't you instead find the last occurrence of . in prefix, the first occurrence of . in subBranchName and then, after matching the substrings, do
fullName = prefix + subBranchName.substr(firstDotIdx);
?
(sorry, I know this is a draft and maybe you are already thinking of changing this code, but just in case..)
|
Just to add that with these changes I see about a factor of 5 improvement in the runnig speed for the initialisation part of an RDF setup with a file with 30k branches, thanks! |
| const std::string prefix{b->GetName()}; | ||
| if (prefix.compare(0, prefix.size(), name, prefix.size()) != 0) | ||
| continue; |
There was a problem hiding this comment.
This is seem odd. Which cases does it support? Does it work correctly if the top level branch is elided from the name search or the branch name (i.e. 'no trailing dot' case)?
There was a problem hiding this comment.
It was a smaller optimization I was playing with before the unordered_map. It probably won't be necessary so I think I will remove it.
| @@ -226,6 +229,8 @@ class TTree : public TNamed, public TAttLine, public TAttFill, public TAttMarker | |||
| }; | |||
|
|
|||
| public: | |||
| void InsertNameAndBranch(std::pair<std::string, TBranch *> &&kv) { fNamesToBranches.insert(kv); } | |||
There was a problem hiding this comment.
This probably should not be public. We may need to have TBranch__SetTree become a friend of TTree.
| void InsertNameAndBranch(std::pair<std::string, TBranch *> &&kv) { fNamesToBranches.insert(kv); } | |
| void RegisterBranchFullName(std::pair<std::string, TBranch *> &&kv) { fNamesToBranches.insert(kv); } |
When the input TTree has a large dataset schema O(10k) and when the branch types are non trivial (i.e. many of the branches store some kind of nested data structure with possibly multiple levels of sub-branches representing the data members), retrieving the full list of available branches in the tree may become a costly operation. Anecdotally, a TTree used by an ATLAS analysis with >30k total leaves results in a retrieval time of around 2 minutes on my machine. With the changes in this PR, this time goes down to around 20 seconds. Main two contributors:
WIP for now, local testing indicates there is a speedup but it needs to be checked with the CI.