[df] Rework RNTuple MT scheduling with a split work-stealing approach - #23327
[df] Rework RNTuple MT scheduling with a split work-stealing approach#23327vepadulano wants to merge 1 commit into
Conversation
Test Results 23 files 23 suites 3d 20h 37m 44s ⏱️ For more details on these failures, see this check. Results for commit ece30c1. ♻️ This comment has been updated with latest results. |
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
🚀
Here's a first pass with a few suggestions and a few questions.
There was a problem hiding this comment.
As a new effort (along with cleaning up ROOT's install tree), I could imagine moving this file to inc/ROOT/Internal.
| friend void ROOT::Internal::IMTUtils::ParallelFor( | ||
| TThreadExecutor &pool, std::shared_ptr<ROOT::Internal::IMTUtils::RParallelSplitFileProcessor> proc, | ||
| const std::function<void(const ROOT::Internal::IMTUtils::RParallelSplitFileProcessor &)> &body); |
There was a problem hiding this comment.
Is the private/friend mechanic required?
If the method were public, you could still not call the overload unless you have a shared_ptr to ROOT::Internal::IMTUtils::RParallelSplitFileProcessor.
| ULong64_t fFirstEntry = 0; ///< First entry index in fSource | ||
| /// End entry index in fSource, e.g. the number of entries in the range is fLastEntry - fFirstEntry | ||
| ULong64_t fLastEntry = 0; | ||
| ULong64_t fEntryOffset = 0; /// Offset of both first and last entries w.r.t. the page source |
There was a problem hiding this comment.
Note that the doxygen comment will pertain to the string_view below. If we are changing it anyway, could you explain why it's needed? Why not add the offset to both numbers and not have this field?
| @@ -86,10 +109,11 @@ class RNTupleDS final : public ROOT::RDF::RDataSource { | |||
| /// The GetEntryRanges() swaps fNextRanges and fCurrentRanges and uses the list of | |||
| /// REntryRangeDS records to return the list of ranges ready to use by the RDF loop manager. | |||
| struct REntryRangeDS { | |||
| std::unique_ptr<ROOT::Internal::RPageSource> fSource; | |||
| std::shared_ptr<ROOT::Internal::RPageSource> fSource; | |||
There was a problem hiding this comment.
Why do the pages sources become shared_ptr?
There was a problem hiding this comment.
Because TBB needs to be able to copy it, it seems.
| if (sampleMap.empty()) | ||
| return ROOT::RDF::RSampleInfo( | ||
| ntupleID, std::make_pair(fCurrentRanges[rangeIdx].fFirstEntry, fCurrentRanges[rangeIdx].fLastEntry), nullptr, | ||
| fPrincipalDescriptor.GetNEntries()); | ||
| return ROOT::RDF::RSampleInfo(ntupleID, | ||
| std::make_pair(entryRangeDS.fFirstEntry - entryRangeDS.fEntryOffset, | ||
| entryRangeDS.fLastEntry - entryRangeDS.fEntryOffset), | ||
| nullptr, fPrincipalDescriptor.GetNEntries()); | ||
|
|
||
| if (sampleMap.find(ntupleID) == sampleMap.end()) | ||
| throw std::runtime_error("Full sample identifier '" + ntupleID + "' cannot be found in the available samples."); | ||
|
|
||
| return ROOT::RDF::RSampleInfo( | ||
| ntupleID, std::make_pair(fCurrentRanges[rangeIdx].fFirstEntry, fCurrentRanges[rangeIdx].fLastEntry), | ||
| sampleMap.at(ntupleID), fPrincipalDescriptor.GetNEntries()); | ||
| return ROOT::RDF::RSampleInfo(ntupleID, | ||
| std::make_pair(entryRangeDS.fFirstEntry - entryRangeDS.fEntryOffset, | ||
| entryRangeDS.fLastEntry - entryRangeDS.fEntryOffset), | ||
| sampleMap.at(ntupleID), fPrincipalDescriptor.GetNEntries()); |
There was a problem hiding this comment.
Initially not part of this PR, but looking at these lines, could we unify them for better readability? I was wondering if all arguments of the constructor for both cases are identical (they pretty much are), but it wasn't so easy to see.
How about this?
RSample* sample = nullptr;
if (!sampleMap.empty()) {
if (auto result = sampleMap.find(ntupleID); result != sampleMap.end())
sample = *result;
else
throw std::runtime_error("Full sample identifier '" + ntupleID + "' cannot be found in the available samples.");
}
return ROOT::RDF::RSampleInfo(ntupleID,
std::make_pair(entryRangeDS.fFirstEntry - entryRangeDS.fEntryOffset,
entryRangeDS.fLastEntry - entryRangeDS.fEntryOffset),
sample, fPrincipalDescriptor.GetNEntries());| newPageSource->SetEntryRange( | ||
| {fClusters[newClusterIdxBegin].fFirstEntry, | ||
| fClusters[newClusterIdxEnd - 1].fLastEntryPlusOne - fClusters[newClusterIdxBegin].fFirstEntry}); |
There was a problem hiding this comment.
Looks like this is doubled. The constructor seems to do the same.
| fClusterIdxBegin(clusterIdxBegin), | ||
| fClusterIdxEnd(clusterIdxEnd) | ||
| { | ||
| fPageSource->SetEntryRange( |
There was a problem hiding this comment.
Should we pageSource->Attach() for consistency / safety?
| auto newClusterIdxBegin = clusterIdxMid; | ||
| auto newClusterIdxEnd = fClusterIdxEnd; | ||
| auto newPageSource = fPageSource->Clone(); | ||
| newPageSource->Attach(); |
There was a problem hiding this comment.
Might not be needed if done in constructor.
| } | ||
| } | ||
|
|
||
| auto processFileWithGlobalOffset = [&](std::size_t fileIdx) { |
| const auto &fileName = fFileNames[fileIdx]; | ||
| auto source = CreatePageSource(fNTupleName, fFileNames[fileIdx]); |
There was a problem hiding this comment.
For consistency in case somebody changes fileName at some point.
| const auto &fileName = fFileNames[fileIdx]; | |
| auto source = CreatePageSource(fNTupleName, fFileNames[fileIdx]); | |
| const auto &fileName = fFileNames[fileIdx]; | |
| auto source = CreatePageSource(fNTupleName, fileName); |
2 participants
Alternative to #22631 , implementing native IMT work-stealing for the RNTuple data source