From 070b2aa005bd1a521748ddc749083a4703681202 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sun, 25 Oct 2026 23:44:20 +0100 Subject: [PATCH 1/5] [ntuple] track pinned cluster groups ...in addition to the pinned clusters that induce pinning of their cluster group. --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 7 +++++-- tree/ntuple/src/RPageStorage.cxx | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 94c3dec969aff..aa7e47cd9103d 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -808,6 +808,9 @@ private: /// Pinned clusters and their $2 * (cluster bunch size) - 1$ successors will not be evicted from the cluster pool. /// Pages of pinned clusters won't be evicted from the page pool. std::unordered_set fPinnedClusters; + /// Reference counted map of pinned cluster groups. This map is edited by PinCluster() and UnpinCluster() + /// and stores the number of pinned clusters in a certain cluster group. + std::unordered_map fPinnedClusterGroups; /// Ensures that fDescriptor has cluster details loaded for the given cluster group ID. This method is expected /// to be called while holding the lock passed by descGuard. The returned descriptor guard is either the @@ -1010,9 +1013,9 @@ public: void UnzipCluster(ROOT::Internal::RCluster *cluster); /// Instructs the cluster pool and page pool to consider the given cluster as active (should stay cached). - void PinCluster(ROOT::DescriptorId_t clusterId) { fPinnedClusters.insert(clusterId); } + void PinCluster(ROOT::DescriptorId_t clusterId); /// Allows the given cluster to be evicted from the cluster pool and page pool. - void UnpinCluster(ROOT::DescriptorId_t clusterId) { fPinnedClusters.erase(clusterId); } + void UnpinCluster(ROOT::DescriptorId_t clusterId); const std::unordered_set &GetPinnedClusters() const { return fPinnedClusters; } // TODO(gparolini): for symmetry with SealPage(), we should either make this private or SealPage() public. diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index caf2ef81650cc..4d54e16c178eb 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -353,6 +353,21 @@ ROOT::NTupleSize_t ROOT::Internal::RPageSource::GetNElements(ROOT::DescriptorId_ return columnRange.GetFirstElementIndex() + columnRange.GetNElements(); } +void ROOT::Internal::RPageSource::PinCluster(ROOT::DescriptorId_t clusterId) +{ + fPinnedClusters.insert(clusterId); + ++fPinnedClusterGroups[FindClusterGroupId(clusterId)]; +} + +void ROOT::Internal::RPageSource::UnpinCluster(ROOT::DescriptorId_t clusterId) +{ + fPinnedClusters.erase(clusterId); + auto itr = fPinnedClusterGroups.find(FindClusterGroupId(clusterId)); + assert(itr != fPinnedClusterGroups.end() && itr->second > 0); + if (--itr->second == 0) + fPinnedClusterGroups.erase(itr); +} + ROOT::Internal::RPageSource::RAnyDescriptorGuard ROOT::Internal::RPageSource::FindNextClusterId(ROOT::DescriptorId_t clusterId, ROOT::DescriptorId_t &nextId) { From 870e6a0349ce1823acb611b6868625634ea584ab Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 26 Oct 2026 14:42:41 +0100 Subject: [PATCH 2/5] [ntuple] refactor element-wise FindClusterId() Move "searching for column index in a given cluster group" and "binary search for the column index in the clusters of a cluster group" into their own helper methods. --- tree/ntuple/src/RPageStorage.cxx | 117 ++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 40 deletions(-) diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 4d54e16c178eb..7e7f487b044de 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -444,6 +444,72 @@ ROOT::Internal::RPageSource::FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::De return descGuard; } +namespace { + +// Searches the index of a column in a certain cluster group. If the column index is found, returns the list +// of cluster IDs in that group. Otherwise, increment or decrement the given clusterGroupId to indicate to which +// side of the cluster groups the search should continue. +const std::vector * +SearchInClusterGroup(ROOT::DescriptorId_t physicalColumnId, ROOT::NTupleSize_t index, + ROOT::DescriptorId_t &clusterGroupId, ROOT::Internal::RPageSource::RAnyDescriptorGuard &descGuard) +{ + const auto &clusterIds = descGuard->GetClusterGroupDescriptor(clusterGroupId).GetClusterIds(); + R__ASSERT(!clusterIds.empty()); + + const auto &firstColumnRange = descGuard->GetClusterDescriptor(clusterIds.front()).GetColumnRange(physicalColumnId); + if (firstColumnRange.GetFirstElementIndex() > index) { + // Look into previous cluster groups + R__ASSERT(clusterGroupId > 0); + clusterGroupId--; + return nullptr; + } + + const auto &lastColumnRange = descGuard->GetClusterDescriptor(clusterIds.back()).GetColumnRange(physicalColumnId); + if ((lastColumnRange.GetFirstElementIndex() + lastColumnRange.GetNElements()) <= index) { + // Look into the following cluster groups (if applicable) + if (++clusterGroupId == descGuard->GetNClusterGroups()) + clusterGroupId = ROOT::kInvalidDescriptorId; + return nullptr; + } + + return &clusterIds; +} + +// Binary search in the sorted list clusterIdList. The caller has to ensure that the index of the given column +// is in fact in the list of provided cluster IDs. +ROOT::DescriptorId_t FindClusterIdInList(ROOT::DescriptorId_t physicalColumnId, ROOT::NTupleSize_t index, + const std::vector &clusterIds, + ROOT::Internal::RPageSource::RAnyDescriptorGuard &descGuard) +{ + std::size_t clusterLeft = 0; + std::size_t clusterRight = clusterIds.size() - 1; + while (clusterLeft <= clusterRight) { + const std::size_t clusterMidpoint = (clusterLeft + clusterRight) / 2; + const auto clusterId = clusterIds[clusterMidpoint]; + const auto &columnRange = descGuard->GetClusterDescriptor(clusterId).GetColumnRange(physicalColumnId); + + if (columnRange.Contains(index)) { + return clusterId; + } + + if (columnRange.GetFirstElementIndex() > index) { + R__ASSERT(clusterMidpoint > 0); + clusterRight = clusterMidpoint - 1; + continue; + } + + if (columnRange.GetFirstElementIndex() + columnRange.GetNElements() <= index) { + clusterLeft = clusterMidpoint + 1; + continue; + } + } + + R__ASSERT(false); + return ROOT::kInvalidDescriptorId; +} + +} // anonymous namespace + ROOT::Internal::RPageSource::RAnyDescriptorGuard ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTupleSize_t index, DescriptorId_t &cid) { @@ -461,52 +527,23 @@ ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTup const std::size_t cgMidpoint = (cgLeft + cgRight) / 2; descGuard = EnsureClusterDetails(cgMidpoint, std::move(descGuard)); - const auto cgIter = descGuard->GetClusterGroupIterable().begin(); - const auto &clusterIds = (cgIter + cgMidpoint)->GetClusterIds(); - R__ASSERT(!clusterIds.empty()); + auto cgHint = cgMidpoint; + const std::vector *clusterIds = SearchInClusterGroup(physicalColumnId, index, cgHint, descGuard); - const auto &firstColumnRange = - descGuard->GetClusterDescriptor(clusterIds.front()).GetColumnRange(physicalColumnId); - if (firstColumnRange.GetFirstElementIndex() > index) { - // Look into the lower half of cluster groups - R__ASSERT(cgMidpoint > 0); - cgRight = cgMidpoint - 1; - continue; + if (clusterIds) { + cid = FindClusterIdInList(physicalColumnId, index, *clusterIds, descGuard); + break; } - const auto &lastColumnRange = descGuard->GetClusterDescriptor(clusterIds.back()).GetColumnRange(physicalColumnId); - if ((lastColumnRange.GetFirstElementIndex() + lastColumnRange.GetNElements()) <= index) { + if (cgHint == ROOT::kInvalidDescriptorId) { + break; + } else if (cgHint < cgMidpoint) { + // Look into the lower half of cluster groups + cgRight = cgMidpoint - 1; + } else { // Look into the upper half of cluster groups cgLeft = cgMidpoint + 1; - continue; } - - // Binary search in the current cluster group; since we already checked the element range boundaries, - // the element must be in that cluster group. - std::size_t clusterLeft = 0; - std::size_t clusterRight = clusterIds.size() - 1; - while (clusterLeft <= clusterRight) { - const std::size_t clusterMidpoint = (clusterLeft + clusterRight) / 2; - const auto clusterId = clusterIds[clusterMidpoint]; - const auto &columnRange = descGuard->GetClusterDescriptor(clusterId).GetColumnRange(physicalColumnId); - - if (columnRange.Contains(index)) { - cid = clusterId; - return descGuard; - } - - if (columnRange.GetFirstElementIndex() > index) { - R__ASSERT(clusterMidpoint > 0); - clusterRight = clusterMidpoint - 1; - continue; - } - - if (columnRange.GetFirstElementIndex() + columnRange.GetNElements() <= index) { - clusterLeft = clusterMidpoint + 1; - continue; - } - } - R__ASSERT(false); } return descGuard; } From d006655f55902ab4a8ff7ece5b4a0ebf9e223c77 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 26 Oct 2026 15:50:15 +0100 Subject: [PATCH 3/5] [ntuple] add shortcuts to element-wise FindClusterId() - Look first into the cluster group of the current cluster and one adjacent cluster group before full binary search. - Look through pinned cluster groups - "Top-level columns" (columns that have one element per entry) can use the more efficient entry-wise FindClusterId() --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 5 +- tree/ntuple/src/RPageStorage.cxx | 70 ++++++++++++++++++++++++++- tree/ntuple/test/ntuple_storage.cxx | 66 ++++++++++++++++++++++--- 3 files changed, 131 insertions(+), 10 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index aa7e47cd9103d..84cee480b0244 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -809,8 +809,9 @@ private: /// Pages of pinned clusters won't be evicted from the page pool. std::unordered_set fPinnedClusters; /// Reference counted map of pinned cluster groups. This map is edited by PinCluster() and UnpinCluster() - /// and stores the number of pinned clusters in a certain cluster group. - std::unordered_map fPinnedClusterGroups; + /// and stores the number of pinned clusters in a certain cluster group. We use a map because we iterate the sorted + /// list of cluster group IDs in the element-wise FindClusterId(). + std::map fPinnedClusterGroups; /// Ensures that fDescriptor has cluster details loaded for the given cluster group ID. This method is expected /// to be called while holding the lock passed by descGuard. The returned descriptor guard is either the diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 7e7f487b044de..2997e010543d8 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -519,7 +519,73 @@ ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTup if (descGuard->GetNClusterGroups() == 0) return descGuard; + // Shortcuts: most cases do not require a full binary search in the list of cluster groups. + + // Case 1: look around the last used cluster + const std::vector *clusterIds = nullptr; + const DescriptorId_t cgGuess = + ((index == 0) || (fLastUsedCluster == kInvalidDescriptorId)) ? 0 : FindClusterGroupId(fLastUsedCluster); + DescriptorId_t cgHint = cgGuess; + // Try twice: if the column index is not found in the cluster group of the last used cluster, cgHint will point + // to the adjacent cluster group either preceeding or following the cluster group and we'll try again. + for (int i = 0; i < 2; ++i) { + descGuard = EnsureClusterDetails(cgHint, std::move(descGuard)); + if ((clusterIds = SearchInClusterGroup(physicalColumnId, index, cgHint, descGuard))) { + cid = FindClusterIdInList(physicalColumnId, index, *clusterIds, descGuard); + return descGuard; + } + if (cgHint == kInvalidDescriptorId) + return descGuard; + } + assert(cgHint != cgGuess); + + // Case 2: look into the pinned cluster groups + // We expect only few pinned cluster groups, so we do a linear search with pruning + const bool isRightSided = cgHint > cgGuess; // whether to look to the right or to the left of cgHint (cgGuess) + for (auto [cgPinned, _] : fPinnedClusterGroups) { + if (isRightSided && (cgPinned < cgHint)) { + // Skip the left side of pinned cluster groups if we already know the column index cannot be here + continue; + } else if (!isRightSided && (cgPinned > cgHint)) { + // Prune the search on the right side of pinned cluster groups + break; + } + + descGuard = EnsureClusterDetails(cgPinned, std::move(descGuard)); + cgHint = cgPinned; + if ((clusterIds = SearchInClusterGroup(physicalColumnId, index, cgHint, descGuard))) { + cid = FindClusterIdInList(physicalColumnId, index, *clusterIds, descGuard); + return descGuard; + } + + if (cgHint < cgPinned) { + // No point in moving further to the right of the pinned cluster groups + break; + } + } + + // Case 3: top-level columns can use the entry-based search + const auto &columnDesc = descGuard->GetColumnDescriptor(physicalColumnId); + if (columnDesc.GetIndex() == 0) { + // Start search with the parent field of the current column + auto fieldId = descGuard->GetFieldDescriptor(columnDesc.GetFieldId()).GetParentId(); + while (fieldId != descGuard->GetFieldZeroId()) { + const auto &fieldDesc = descGuard->GetFieldDescriptor(fieldId); + if ((fieldDesc.GetStructure() != ENTupleStructure::kPlain) && + (fieldDesc.GetStructure() != ENTupleStructure::kRecord)) { + break; + } + fieldId = fieldDesc.GetParentId(); + } + if (fieldId == descGuard->GetFieldZeroId()) { + descGuard.Release(); + return FindClusterId(index, cid); + } + } + // Binary search in the cluster group list, followed by a binary search in the clusters of that cluster group + // We could use the information from the shortcut search to restrict the left and right borders further. + // Left out for simplicity. std::size_t cgLeft = 0; std::size_t cgRight = descGuard->GetNClusterGroups() - 1; @@ -527,8 +593,8 @@ ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTup const std::size_t cgMidpoint = (cgLeft + cgRight) / 2; descGuard = EnsureClusterDetails(cgMidpoint, std::move(descGuard)); - auto cgHint = cgMidpoint; - const std::vector *clusterIds = SearchInClusterGroup(physicalColumnId, index, cgHint, descGuard); + cgHint = cgMidpoint; + clusterIds = SearchInClusterGroup(physicalColumnId, index, cgHint, descGuard); if (clusterIds) { cid = FindClusterIdInList(physicalColumnId, index, *clusterIds, descGuard); diff --git a/tree/ntuple/test/ntuple_storage.cxx b/tree/ntuple/test/ntuple_storage.cxx index da8c0156daf21..5619470014804 100644 --- a/tree/ntuple/test/ntuple_storage.cxx +++ b/tree/ntuple/test/ntuple_storage.cxx @@ -1321,11 +1321,12 @@ TEST(RPageSource, MetadataMode) FileRaii fileGuard("test_ntuple_page_source_metadata_mode.root"); auto model = RNTupleModel::Create(); - auto ptrCtr = model->MakeField("ctr"); + auto ptrCtr = model->MakeField>("ctr"); + ptrCtr->resize(1); { auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); for (int i = 0; i < 1000; ++i) { - *ptrCtr = i; + (*ptrCtr)[0] = i; writer->Fill(); writer->CommitCluster(true /* commit cluster group */); } @@ -1350,13 +1351,66 @@ TEST(RPageSource, MetadataMode) EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusters()); EXPECT_EQ(0u, reader->GetDescriptor().GetNActiveClusters()); - ptrCtr = reader->GetModel().GetDefaultEntry().GetPtr("ctr"); + ptrCtr = reader->GetModel().GetDefaultEntry().GetPtr>("ctr"); reader->LoadEntry(500); - EXPECT_EQ(500, *ptrCtr); + EXPECT_EQ(500, (*ptrCtr)[0]); reader->LoadEntry(999); - EXPECT_EQ(999, *ptrCtr); + EXPECT_EQ(999, (*ptrCtr)[0]); reader->LoadEntry(0); - EXPECT_EQ(0, *ptrCtr); + EXPECT_EQ(0, (*ptrCtr)[0]); EXPECT_LT(0, reader->GetDescriptor().GetNActiveClusters()); EXPECT_GT(1000, reader->GetDescriptor().GetNActiveClusters()); + + reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); + { + // Check that we don't do full binary search on the cluster groups for sequential iteration + auto viewInt = reader->GetView("ctr._0"); + for (unsigned int i = 0; i < 10; ++i) { + EXPECT_EQ(i, viewInt(i)); + } + EXPECT_LT(0, reader->GetDescriptor().GetNActiveClusters()); + EXPECT_GT(15, reader->GetDescriptor().GetNActiveClusters()); + } + + reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); + { + // Check that we don't do full binary search on the cluster groups for multi-stream iteration + auto viewInt = reader->GetView("ctr._0"); + auto stream1 = reader->CreateActiveEntryToken(); + auto stream2 = reader->CreateActiveEntryToken(); + for (unsigned int i = 0; i < 10; ++i) { + if (i >= 5) { + stream2.SetEntryNumber(i - 5); + EXPECT_EQ(i - 5, viewInt(i - 5)); + } + stream1.SetEntryNumber(i); + EXPECT_EQ(i, viewInt(i)); + } + EXPECT_LT(0, reader->GetDescriptor().GetNActiveClusters()); + EXPECT_GT(15, reader->GetDescriptor().GetNActiveClusters()); + } + + reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); + { + // Check that random access on top-level fields does not trigger full binary search in cluster groups + auto viewCtr = reader->GetView>("ctr"); + // We will initially look into the first cluster (group). Then hit the cluster groups exactly. We also + // prefetch up to two clusters per read. + viewCtr(500); + viewCtr(300); + + EXPECT_LT(0, reader->GetDescriptor().GetNActiveClusters()); + EXPECT_GE(9, reader->GetDescriptor().GetNActiveClusters()); + } + + reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); + { + // Cross-check: random access on collection columns actually do require full binary search in the cluster groups + // (but not worse) + auto viewInt = reader->GetView("ctr._0"); + viewInt(500); + + EXPECT_LT(10, reader->GetDescriptor().GetNActiveClusters()); + EXPECT_GT(20, reader->GetDescriptor().GetNActiveClusters()); + } } From 54fb0d7fa711656ebf470177d42c930ca37e7b9a Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 28 Oct 2026 09:26:09 +0100 Subject: [PATCH 4/5] [ntuple] fix limits unit test for on-demand page list loading --- tree/ntuple/test/ntuple_limits.cxx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tree/ntuple/test/ntuple_limits.cxx b/tree/ntuple/test/ntuple_limits.cxx index ed769ae4b93f4..15ef0a3b3e336 100644 --- a/tree/ntuple/test/ntuple_limits.cxx +++ b/tree/ntuple/test/ntuple_limits.cxx @@ -241,6 +241,7 @@ TEST(RNTuple, DISABLED_Limits_LargePage) RNTupleReadOptions options; options.SetClusterCache(RNTupleReadOptions::EClusterCache::kOff); + options.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath(), options); const auto &descriptor = reader->GetDescriptor(); const auto &model = reader->GetModel(); @@ -286,7 +287,9 @@ TEST(RNTuple, DISABLED_Limits_LargePageOneEntry) writer->Fill(); } - auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath()); + RNTupleReadOptions options; + options.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath(), options); const auto &descriptor = reader->GetDescriptor(); const auto &model = reader->GetModel(); auto fieldId = descriptor.FindFieldId("ids"); From 1575491d19e6b1502f936141a936feb7b254cbca Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 29 Oct 2026 23:21:08 +0100 Subject: [PATCH 5/5] [ntuple] introduce cluster group budget Limit the number of concurrently loaded cluster group details. The corresponding read options stays internal for the time being. --- tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx | 16 +++++++ tree/ntuple/inc/ROOT/RPageStorage.hxx | 4 ++ tree/ntuple/src/RPageStorage.cxx | 49 +++++++++++++++++++-- tree/ntuple/test/ntuple_limits.cxx | 6 ++- tree/ntuple/test/ntuple_storage.cxx | 40 +++++++++++++++++ 5 files changed, 111 insertions(+), 4 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx b/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx index 807afa7046642..96cd89d093e9f 100644 --- a/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx +++ b/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx @@ -25,6 +25,8 @@ class RNTupleReadOptionsManip final { public: static unsigned int GetClusterBunchSize(const RNTupleReadOptions &options); static void SetClusterBunchSize(RNTupleReadOptions &options, unsigned int val); + static unsigned int GetClusterGroupBudget(const RNTupleReadOptions &options); + static void SetClusterGroupBudget(RNTupleReadOptions &options, unsigned int val); static std::uint64_t GetMaxEnvelopeSize(const RNTupleReadOptions &options); static void SetMaxEnvelopeSize(RNTupleReadOptions &options, std::uint64_t val); }; @@ -114,6 +116,10 @@ private: /// The number of cluster to be prefetched in a single batch; this option is transitional and will be replaced /// by an option that allows to control the amount of memory that the prefetcher uses. unsigned int fClusterBunchSize = 1; + /// The threshold for the number of concurrently loaded cluster group details as of which the cluster details of the + /// least recently used cluster groups are evicted. The budget can be overspent due to pinned cluster groups. + /// Currently a private option that cannot be changed by the user. + unsigned int fClusterGroupBudget = 2; EImplicitMT fUseImplicitMT = EImplicitMT::kDefault; EMetadataMode fMetadataMode = EMetadataMode::kDefault; bool fEnableMetrics = false; @@ -147,6 +153,16 @@ inline void RNTupleReadOptionsManip::SetClusterBunchSize(RNTupleReadOptions &opt options.fClusterBunchSize = val; } +inline unsigned int RNTupleReadOptionsManip::GetClusterGroupBudget(const RNTupleReadOptions &options) +{ + return options.fClusterGroupBudget; +} + +inline void RNTupleReadOptionsManip::SetClusterGroupBudget(RNTupleReadOptions &options, unsigned int val) +{ + options.fClusterGroupBudget = val; +} + inline std::uint64_t RNTupleReadOptionsManip::GetMaxEnvelopeSize(const RNTupleReadOptions &options) { return options.fMaxEnvelopeSize; diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 84cee480b0244..d96761db74d7b 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -812,6 +812,10 @@ private: /// and stores the number of pinned clusters in a certain cluster group. We use a map because we iterate the sorted /// list of cluster group IDs in the element-wise FindClusterId(). std::map fPinnedClusterGroups; + /// List of loaded cluster groups, ordered by last access. The size of the list is usually bounded by the + /// page list budget in the read options. Pinned cluster groups can cause overspending of the budget. + /// We except a small number of loaded cluster groups, thus we accept O(n) complexity to edit this list. + std::deque fClusterGroupAccessSequence; /// Ensures that fDescriptor has cluster details loaded for the given cluster group ID. This method is expected /// to be called while holding the lock passed by descGuard. The returned descriptor guard is either the diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 2997e010543d8..73efa10526918 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -268,6 +268,9 @@ void ROOT::Internal::RPageSource::LoadAllPageLists() for (const auto &cgDesc : descGuard->GetClusterGroupIterable()) { LoadPageList(cgDesc.GetId(), descGuard); } + // By clearing the access sequence with all page lists loaded, we make EnsureClusterDetails() always take the + // fast path that doesn't try further maintaining the list. + fClusterGroupAccessSequence.clear(); } void ROOT::Internal::RPageSource::LoadPageList(DescriptorId_t clusterGroupId, const RExclDescriptorGuard &exclGuard) @@ -303,19 +306,59 @@ ROOT::Internal::RPageSource::RAnyDescriptorGuard ROOT::Internal::RPageSource::EnsureClusterDetails(DescriptorId_t cgId, RAnyDescriptorGuard descGuard) { assert(descGuard.IsValid()); - if ((cgId == kInvalidDescriptorId) || descGuard->GetClusterGroupDescriptor(cgId).HasClusterDetails()) + if (cgId == kInvalidDescriptorId) return descGuard; + // After LoadAllPages(), itrSeq == fClusterGroupAccessSequence.begin() == fClusterGroupAccessSequence.end() + auto itrSeq = std::find(fClusterGroupAccessSequence.begin(), fClusterGroupAccessSequence.end(), cgId); + + if (descGuard->GetClusterGroupDescriptor(cgId).HasClusterDetails() && + (itrSeq == fClusterGroupAccessSequence.begin())) { + return descGuard; + } + descGuard.Release(); auto exclGuard = GetExclDescriptorGuard(); + itrSeq = std::find(fClusterGroupAccessSequence.begin(), fClusterGroupAccessSequence.end(), cgId); + if (exclGuard->GetClusterGroupDescriptor(cgId).HasClusterDetails()) { - // unlikely, but between releasing the shared guard and acquiring the exclusive guard, the cluster details - // may have appeared + // Either between releasing the shared guard and acquiring the exclusive guard the cluster group details appeared + // or the cluster group details were present initially but we need to update the access sequence. + assert(itrSeq != fClusterGroupAccessSequence.end()); + fClusterGroupAccessSequence.erase(itrSeq); + fClusterGroupAccessSequence.push_front(cgId); return exclGuard; } + assert(itrSeq == fClusterGroupAccessSequence.end()); + const auto nClusterGroups = fClusterGroupAccessSequence.size(); + const auto clusterGroupBudget = Internal::RNTupleReadOptionsManip::GetClusterGroupBudget(fOptions); + assert(clusterGroupBudget > 0); + if ((nClusterGroups >= clusterGroupBudget) && (nClusterGroups > fPinnedClusterGroups.size())) { + // Remove as many cluster group details as necessary to allow for adding one more and still staying within + // the budget. Take into account, however, that pinned cluster groups cannot be removed. + auto nRemove = std::min(nClusterGroups - fPinnedClusterGroups.size(), nClusterGroups - clusterGroupBudget + 1); + // We iterate from the back to the front of the list, in reverse order of last access (i.e.: starting from the + // least recently used), and we skip pinned cluster groups. + for (auto lru = fClusterGroupAccessSequence.rbegin(); nRemove > 0;) { + if (fPinnedClusterGroups.count(*lru) > 0) { + ++lru; + continue; + } + exclGuard->DropClusterGroupDetails(*lru); + // The forward iterator corresponding to the reverse iterator lru is prev(lru.base). + // After removal, next points to the element following the removed one. All other iterators are invalid. + auto next = fClusterGroupAccessSequence.erase(std::prev(lru.base())); + // make_reverse_iterator brings lru back to the element before the removed one + lru = std::make_reverse_iterator(next); + nRemove--; + }; + assert(fClusterGroupAccessSequence.size() > 0); + } + fClusterGroupAccessSequence.push_front(cgId); + LoadPageList(cgId, exclGuard); return exclGuard; diff --git a/tree/ntuple/test/ntuple_limits.cxx b/tree/ntuple/test/ntuple_limits.cxx index 15ef0a3b3e336..184ebda40f822 100644 --- a/tree/ntuple/test/ntuple_limits.cxx +++ b/tree/ntuple/test/ntuple_limits.cxx @@ -106,7 +106,11 @@ TEST(RNTuple, Limits_ManyClusterGroups) } } - auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath()); + ROOT::RNTupleReadOptions options; + // Prevent page list thrashing: since every entry/cluster is in its own cluster group, with prefetching we have + // 3 active cluster groups at all times. + ROOT::Internal::RNTupleReadOptionsManip::SetClusterGroupBudget(options, 3); + auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath(), options); const auto &descriptor = reader->GetDescriptor(); const auto &model = reader->GetModel(); diff --git a/tree/ntuple/test/ntuple_storage.cxx b/tree/ntuple/test/ntuple_storage.cxx index 5619470014804..c59e97b5dc3d0 100644 --- a/tree/ntuple/test/ntuple_storage.cxx +++ b/tree/ntuple/test/ntuple_storage.cxx @@ -1334,6 +1334,7 @@ TEST(RPageSource, MetadataMode) RNTupleReadOptions opts; opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + ROOT::Internal::RNTupleReadOptionsManip::SetClusterGroupBudget(opts, 100); // make sure page lists are not evicted auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusterGroups()); EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusters()); @@ -1414,3 +1415,42 @@ TEST(RPageSource, MetadataMode) EXPECT_GT(20, reader->GetDescriptor().GetNActiveClusters()); } } + +TEST(RPageSource, ClusterGroupBudget) +{ + FileRaii fileGuard("test_ntuple_page_source_cluster_group_bedget.root"); + + auto model = RNTupleModel::Create(); + auto ptrCtr = model->MakeField>("ctr"); + ptrCtr->resize(1); + { + auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); + for (int c = 0; c < 10; ++c) { + for (int i = 0; i < 10; ++i) { + (*ptrCtr)[0] = 10 * c + i; + writer->Fill(); + writer->CommitCluster((i == 9)); + } + } + } + + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + for (const auto idx : *reader) { + reader->LoadEntry(idx); + } + EXPECT_EQ(20u, reader->GetDescriptor().GetNActiveClusters()); + + auto token1 = reader->CreateActiveEntryToken(); + token1.SetEntryNumber(5); + for (const auto idx : *reader) { + reader->LoadEntry(idx); + } + EXPECT_EQ(20u, reader->GetDescriptor().GetNActiveClusters()); + + auto token2 = reader->CreateActiveEntryToken(); + token2.SetEntryNumber(50); + for (const auto idx : *reader) { + reader->LoadEntry(idx); + } + EXPECT_EQ(30u, reader->GetDescriptor().GetNActiveClusters()); +}