From beb5dc79bd31bfea933a32c2b8fa8e502e4dc614 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 20 Nov 2024 11:53:31 +0100 Subject: [PATCH 1/8] [ntuple] sort cluster groups in descriptor --- tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx | 4 ++++ tree/ntuple/v7/src/RNTupleDescriptor.cxx | 11 ++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx index eb2df2b45d05a..3084022d20c68 100644 --- a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx @@ -561,6 +561,10 @@ private: std::unordered_map fFieldDescriptors; std::unordered_map fColumnDescriptors; std::unordered_map fClusterGroupDescriptors; + /// References cluster groups sorted by entry range and thus allows for binary search. + /// Note that this list is empty during the descriptor building process and will only be + /// created when the final descriptor is extracted from the builder. + std::vector fSortedClusterGroupIds; /// May contain only a subset of all the available clusters, e.g. the clusters of the current file /// from a chain of files std::unordered_map fClusterDescriptors; diff --git a/tree/ntuple/v7/src/RNTupleDescriptor.cxx b/tree/ntuple/v7/src/RNTupleDescriptor.cxx index 94ca004ef79ac..b9027926a85e5 100644 --- a/tree/ntuple/v7/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/v7/src/RNTupleDescriptor.cxx @@ -397,7 +397,6 @@ ROOT::Experimental::RNTupleDescriptor::FindNextClusterId(DescriptorId_t clusterI return kInvalidDescriptorId; } -// TODO(jblomer): fix for cases of sharded clasters ROOT::Experimental::DescriptorId_t ROOT::Experimental::RNTupleDescriptor::FindPrevClusterId(DescriptorId_t clusterId) const { @@ -554,6 +553,7 @@ std::unique_ptr ROOT::Experimental::RNTup clone->fColumnDescriptors.emplace(d.first, d.second.Clone()); for (const auto &d : fClusterGroupDescriptors) clone->fClusterGroupDescriptors.emplace(d.first, d.second.Clone()); + clone->fSortedClusterGroupIds = fSortedClusterGroupIds; for (const auto &d : fClusterDescriptors) clone->fClusterDescriptors.emplace(d.first, d.second.Clone()); for (const auto &d : fExtraTypeInfoDescriptors) @@ -827,6 +827,15 @@ ROOT::Experimental::RResult ROOT::Experimental::Internal::RNTupleDescripto ROOT::Experimental::RNTupleDescriptor ROOT::Experimental::Internal::RNTupleDescriptorBuilder::MoveDescriptor() { EnsureValidDescriptor().ThrowOnError(); + fDescriptor.fSortedClusterGroupIds.reserve(fDescriptor.fClusterGroupDescriptors.size()); + for (const auto &[id, _] : fDescriptor.fClusterGroupDescriptors) + fDescriptor.fSortedClusterGroupIds.emplace_back(id); + std::sort(fDescriptor.fSortedClusterGroupIds.begin(), fDescriptor.fSortedClusterGroupIds.end(), + [this](DescriptorId_t a, DescriptorId_t b) + { + return fDescriptor.fClusterGroupDescriptors[a].GetMinEntry() < + fDescriptor.fClusterGroupDescriptors[b].GetMinEntry(); + }); RNTupleDescriptor result; std::swap(result, fDescriptor); return result; From 0c0ae40efb384207e40054e1e812286b4c6aa5c5 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 20 Nov 2024 12:50:30 +0100 Subject: [PATCH 2/8] [ntuple] sort clusters in cluster group descriptor --- tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx | 3 ++- tree/ntuple/v7/src/RNTupleDescriptor.cxx | 7 ++++++- tree/ntuple/v7/src/RPageStorage.cxx | 2 +- tree/ntuple/v7/test/ntuple_serialize.cxx | 10 +++++----- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx index 3084022d20c68..eaf4bf1392b0c 100644 --- a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx @@ -427,6 +427,7 @@ class RClusterGroupDescriptor { private: DescriptorId_t fClusterGroupId = kInvalidDescriptorId; /// The cluster IDs can be empty if the corresponding page list is not loaded. + /// Otherwise, cluster ids are sorted by first entry number. std::vector fClusterIds; /// The page list that corresponds to the cluster group RNTupleLocator fPageListLocator; @@ -1306,7 +1307,7 @@ public: fClusterGroup.fNClusters = nClusters; return *this; } - void AddClusters(const std::vector &clusterIds) + void AddSortedClusters(const std::vector &clusterIds) { if (clusterIds.size() != fClusterGroup.GetNClusters()) throw RException(R__FAIL("mismatch of number of clusters")); diff --git a/tree/ntuple/v7/src/RNTupleDescriptor.cxx b/tree/ntuple/v7/src/RNTupleDescriptor.cxx index b9027926a85e5..028c37e45525e 100644 --- a/tree/ntuple/v7/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/v7/src/RNTupleDescriptor.cxx @@ -488,8 +488,13 @@ ROOT::Experimental::RNTupleDescriptor::AddClusterGroupDetails(DescriptorId_t clu return R__FAIL("invalid attempt to re-populate existing cluster"); } } + std::sort(clusterIds.begin(), clusterIds.end(), + [this](DescriptorId_t a, DescriptorId_t b) + { + return fClusterDescriptors[a].GetFirstEntryIndex() < fClusterDescriptors[b].GetFirstEntryIndex(); + }); auto cgBuilder = Internal::RClusterGroupDescriptorBuilder::FromSummary(iter->second); - cgBuilder.AddClusters(clusterIds); + cgBuilder.AddSortedClusters(clusterIds); iter->second = cgBuilder.MoveDescriptor().Unwrap(); return RResult::Success(); } diff --git a/tree/ntuple/v7/src/RPageStorage.cxx b/tree/ntuple/v7/src/RPageStorage.cxx index 515ca08668506..8ad3a908d30e2 100644 --- a/tree/ntuple/v7/src/RPageStorage.cxx +++ b/tree/ntuple/v7/src/RPageStorage.cxx @@ -1160,7 +1160,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitClusterGroup() for (auto i = fNextClusterInGroup; i < nClusters; ++i) { clusterIds.emplace_back(i); } - cgBuilder.AddClusters(clusterIds); + cgBuilder.AddSortedClusters(clusterIds); fDescriptorBuilder.AddClusterGroup(cgBuilder.MoveDescriptor().Unwrap()); fSerializationContext.MapClusterGroupId(clusterGroupId); diff --git a/tree/ntuple/v7/test/ntuple_serialize.cxx b/tree/ntuple/v7/test/ntuple_serialize.cxx index 10d66c943cd8a..b0a9824f9b7f3 100644 --- a/tree/ntuple/v7/test/ntuple_serialize.cxx +++ b/tree/ntuple/v7/test/ntuple_serialize.cxx @@ -697,7 +697,7 @@ TEST(RNTuple, SerializeFooter) cgLocator.fBytesOnStorage = 42; cgBuilder.ClusterGroupId(256).PageListLength(137).PageListLocator(cgLocator).NClusters(1).EntrySpan(100); std::vector clusterIds{84}; - cgBuilder.AddClusters(clusterIds); + cgBuilder.AddSortedClusters(clusterIds); builder.AddClusterGroup(cgBuilder.MoveDescriptor().Unwrap()); auto desc = builder.MoveDescriptor(); @@ -1026,7 +1026,7 @@ TEST(RNTuple, SerializeMultiColumnRepresentation) RClusterGroupDescriptorBuilder cgBuilder; cgBuilder.ClusterGroupId(137).NClusters(2).EntrySpan(2); - cgBuilder.AddClusters({13, 17}); + cgBuilder.AddSortedClusters({13, 17}); builder.AddClusterGroup(cgBuilder.MoveDescriptor().Unwrap()); auto desc = builder.MoveDescriptor(); @@ -1212,7 +1212,7 @@ TEST(RNTuple, SerializeMultiColumnRepresentationProjection) RClusterGroupDescriptorBuilder cgBuilder; cgBuilder.ClusterGroupId(137).NClusters(2).EntrySpan(2); - cgBuilder.AddClusters({13, 17}); + cgBuilder.AddSortedClusters({13, 17}); builder.AddClusterGroup(cgBuilder.MoveDescriptor().Unwrap()); auto desc = builder.MoveDescriptor(); @@ -1337,7 +1337,7 @@ TEST(RNTuple, SerializeMultiColumnRepresentationDeferred) RClusterGroupDescriptorBuilder cgBuilder; cgBuilder.ClusterGroupId(137).NClusters(3).EntrySpan(4); - cgBuilder.AddClusters({13, 17, 19}); + cgBuilder.AddSortedClusters({13, 17, 19}); builder.AddClusterGroup(cgBuilder.MoveDescriptor().Unwrap()); auto desc = builder.MoveDescriptor(); @@ -1461,7 +1461,7 @@ TEST(RNTuple, SerializeMultiColumnRepresentationIncremental) RClusterGroupDescriptorBuilder cgBuilder; cgBuilder.ClusterGroupId(137).NClusters(2).EntrySpan(2); - cgBuilder.AddClusters({13, 17}); + cgBuilder.AddSortedClusters({13, 17}); builder.AddClusterGroup(cgBuilder.MoveDescriptor().Unwrap()); auto desc = builder.MoveDescriptor(); From bf99fa14c59c453174832306870f24ef47116dc7 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 20 Nov 2024 15:59:21 +0100 Subject: [PATCH 3/8] [ntuple] improve FindClusterId() complexity from O(n) to O(logn) --- tree/ntuple/v7/src/RNTupleDescriptor.cxx | 57 +++++++++++++++++++++--- 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/tree/ntuple/v7/src/RNTupleDescriptor.cxx b/tree/ntuple/v7/src/RNTupleDescriptor.cxx index 028c37e45525e..c6968168fa8a7 100644 --- a/tree/ntuple/v7/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/v7/src/RNTupleDescriptor.cxx @@ -372,13 +372,58 @@ ROOT::Experimental::RNTupleDescriptor::FindPhysicalColumnId(DescriptorId_t field ROOT::Experimental::DescriptorId_t ROOT::Experimental::RNTupleDescriptor::FindClusterId(DescriptorId_t physicalColumnId, NTupleSize_t index) const { - // TODO(jblomer): binary search? - for (const auto &cd : fClusterDescriptors) { - if (!cd.second.ContainsColumn(physicalColumnId)) + if (GetNClusterGroups() == 0) + return kInvalidDescriptorId; + + // Binary search in the cluster group list, followed by a binary search in the clusters of that cluster group + + std::size_t cgLeft = 0; + std::size_t cgRight = GetNClusterGroups() - 1; + while (cgLeft <= cgRight) { + const std::size_t cgMidpoint = (cgLeft + cgRight) / 2; + const auto &clusterIds = GetClusterGroupDescriptor(fSortedClusterGroupIds[cgMidpoint]).GetClusterIds(); + R__ASSERT(!clusterIds.empty()); + + const auto firstElementInGroup = + GetClusterDescriptor(clusterIds.front()).GetColumnRange(physicalColumnId).fFirstElementIndex; + if (firstElementInGroup > index) { + // Look into the lower half of cluster groups + R__ASSERT(cgMidpoint > 0); + cgRight = cgMidpoint - 1; continue; - auto columnRange = cd.second.GetColumnRange(physicalColumnId); - if (columnRange.Contains(index)) - return cd.second.GetId(); + } + + const auto &lastColumnRange = GetClusterDescriptor(clusterIds.back()).GetColumnRange(physicalColumnId); + if ((lastColumnRange.fFirstElementIndex + lastColumnRange.fNElements) <= index) { + // 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 = GetClusterDescriptor(clusterId).GetColumnRange(physicalColumnId); + + if (columnRange.Contains(index)) + return clusterId; + + if (columnRange.fFirstElementIndex > index) { + R__ASSERT(clusterMidpoint > 0); + clusterRight = clusterMidpoint - 1; + continue; + } + + if (columnRange.fFirstElementIndex + columnRange.fNElements <= index) { + clusterLeft = clusterMidpoint + 1; + continue; + } + } + R__ASSERT(false); } return kInvalidDescriptorId; } From 31f1d94c6e5875129bd7f43b2549fd04a40fa7e6 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 20 Nov 2024 20:46:04 +0100 Subject: [PATCH 4/8] [ntuple] improve Find[Next|Prev]ClusterId() complexity from O(n) to O(logn) --- tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx | 3 + tree/ntuple/v7/src/RNTupleDescriptor.cxx | 71 +++++++++++++++---- tree/ntuple/v7/test/ntuple_cluster.cxx | 10 ++- 3 files changed, 64 insertions(+), 20 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx index eaf4bf1392b0c..eea71cce88a86 100644 --- a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx @@ -572,6 +572,9 @@ private: std::vector fExtraTypeInfoDescriptors; std::unique_ptr fHeaderExtension; + // We don't expose this publicy because when we add sharded clusters, this interface does not make sense anymore + DescriptorId_t FindClusterId(NTupleSize_t entryIdx) const; + public: static constexpr unsigned int kFeatureFlagTest = 137; // Bit reserved for forward-compatibility testing diff --git a/tree/ntuple/v7/src/RNTupleDescriptor.cxx b/tree/ntuple/v7/src/RNTupleDescriptor.cxx index c6968168fa8a7..0ac411aeeb867 100644 --- a/tree/ntuple/v7/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/v7/src/RNTupleDescriptor.cxx @@ -428,30 +428,73 @@ ROOT::Experimental::RNTupleDescriptor::FindClusterId(DescriptorId_t physicalColu return kInvalidDescriptorId; } -// TODO(jblomer): fix for cases of sharded clasters +ROOT::Experimental::DescriptorId_t ROOT::Experimental::RNTupleDescriptor::FindClusterId(NTupleSize_t entryIdx) const +{ + if (GetNClusterGroups() == 0) + return kInvalidDescriptorId; + + // Binary search in the cluster group list, followed by a binary search in the clusters of that cluster group + + std::size_t cgLeft = 0; + std::size_t cgRight = GetNClusterGroups() - 1; + while (cgLeft <= cgRight) { + const std::size_t cgMidpoint = (cgLeft + cgRight) / 2; + const auto &cgDesc = GetClusterGroupDescriptor(fSortedClusterGroupIds[cgMidpoint]); + + if (cgDesc.GetMinEntry() > entryIdx) { + R__ASSERT(cgMidpoint > 0); + cgRight = cgMidpoint - 1; + continue; + } + + if (cgDesc.GetMinEntry() + cgDesc.GetEntrySpan() <= entryIdx) { + 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. + const auto &clusterIds = cgDesc.GetClusterIds(); + R__ASSERT(!clusterIds.empty()); + 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 &clusterDesc = GetClusterDescriptor(clusterIds[clusterMidpoint]); + + if (clusterDesc.GetFirstEntryIndex() > entryIdx) { + R__ASSERT(clusterMidpoint > 0); + clusterRight = clusterMidpoint - 1; + continue; + } + + if (clusterDesc.GetFirstEntryIndex() + clusterDesc.GetNEntries() <= entryIdx) { + clusterLeft = clusterMidpoint + 1; + continue; + } + + return clusterIds[clusterMidpoint]; + } + R__ASSERT(false); + } + return kInvalidDescriptorId; +} + ROOT::Experimental::DescriptorId_t ROOT::Experimental::RNTupleDescriptor::FindNextClusterId(DescriptorId_t clusterId) const { const auto &clusterDesc = GetClusterDescriptor(clusterId); - auto firstEntryInNextCluster = clusterDesc.GetFirstEntryIndex() + clusterDesc.GetNEntries(); - // TODO(jblomer): binary search? - for (const auto &cd : fClusterDescriptors) { - if (cd.second.GetFirstEntryIndex() == firstEntryInNextCluster) - return cd.second.GetId(); - } - return kInvalidDescriptorId; + const auto firstEntryInNextCluster = clusterDesc.GetFirstEntryIndex() + clusterDesc.GetNEntries(); + return FindClusterId(firstEntryInNextCluster); } ROOT::Experimental::DescriptorId_t ROOT::Experimental::RNTupleDescriptor::FindPrevClusterId(DescriptorId_t clusterId) const { const auto &clusterDesc = GetClusterDescriptor(clusterId); - // TODO(jblomer): binary search? - for (const auto &cd : fClusterDescriptors) { - if (cd.second.GetFirstEntryIndex() + cd.second.GetNEntries() == clusterDesc.GetFirstEntryIndex()) - return cd.second.GetId(); - } - return kInvalidDescriptorId; + if (clusterDesc.GetFirstEntryIndex() == 0) + return kInvalidDescriptorId; + return FindClusterId(clusterDesc.GetFirstEntryIndex() - 1); } std::vector diff --git a/tree/ntuple/v7/test/ntuple_cluster.cxx b/tree/ntuple/v7/test/ntuple_cluster.cxx index 790c3813bbe57..528665ae55799 100644 --- a/tree/ntuple/v7/test/ntuple_cluster.cxx +++ b/tree/ntuple/v7/test/ntuple_cluster.cxx @@ -62,12 +62,10 @@ class RPageSourceMock : public RPageSource { .MoveDescriptor() .Unwrap()); } - descBuilder.AddClusterGroup(ROOT::Experimental::Internal::RClusterGroupDescriptorBuilder() - .ClusterGroupId(0) - .MinEntry(0) - .EntrySpan(6) - .MoveDescriptor() - .Unwrap()); + ROOT::Experimental::Internal::RClusterGroupDescriptorBuilder cgBuilder; + cgBuilder.ClusterGroupId(0).MinEntry(0).EntrySpan(6).NClusters(6); + cgBuilder.AddSortedClusters({0, 1, 2, 3, 4, 5}); + descBuilder.AddClusterGroup(cgBuilder.MoveDescriptor().Unwrap()); auto descriptorGuard = GetExclDescriptorGuard(); descriptorGuard.MoveIn(descBuilder.MoveDescriptor()); } From 0f6cc50541ce6c3b80e6618622fd5f2b7d73a682 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 21 Nov 2024 00:47:38 +0100 Subject: [PATCH 5/8] [ntuple] improve RPageRange::Find() complexity from O(n) to O(logn) --- tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx | 5 +++ tree/ntuple/v7/src/RNTupleDescriptor.cxx | 42 +++++++++++++------ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx index eea71cce88a86..85cd22b6b5a4e 100644 --- a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx @@ -279,6 +279,10 @@ public: std::size_t ExtendToFitColumnRange(const RColumnRange &columnRange, const Internal::RColumnElementBase &element, std::size_t pageSize); + /// Has the same length than fPageInfos and stores the sum of the number of elements of all the pages + /// up to and including a given index. Used for binary search in Find(). + std::vector fCumulativeNElements; + public: /// We do not need to store the element size / uncompressed page size because we know to which column /// the page belongs @@ -319,6 +323,7 @@ public: RPageRange clone; clone.fPhysicalColumnId = fPhysicalColumnId; clone.fPageInfos = fPageInfos; + clone.fCumulativeNElements = fCumulativeNElements; return clone; } diff --git a/tree/ntuple/v7/src/RNTupleDescriptor.cxx b/tree/ntuple/v7/src/RNTupleDescriptor.cxx index 0ac411aeeb867..ddf1c29c76b6e 100644 --- a/tree/ntuple/v7/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/v7/src/RNTupleDescriptor.cxx @@ -179,21 +179,32 @@ ROOT::Experimental::RColumnDescriptor ROOT::Experimental::RColumnDescriptor::Clo ROOT::Experimental::RClusterDescriptor::RPageRange::RPageInfoExtended ROOT::Experimental::RClusterDescriptor::RPageRange::Find(ClusterSize_t::ValueType idxInCluster) const { - // TODO(jblomer): binary search - RPageInfo pageInfo; - decltype(idxInCluster) firstInPage = 0; - NTupleSize_t pageNo = 0; - for (const auto &pi : fPageInfos) { - if (firstInPage + pi.fNElements > idxInCluster) { - pageInfo = pi; - break; + const auto N = fCumulativeNElements.size(); + R__ASSERT(N > 0); + R__ASSERT(N == fPageInfos.size()); + + std::size_t left = 0; + std::size_t right = N - 1; + std::size_t midpoint = N; + while (left <= right) { + midpoint = (left + right) / 2; + if (fCumulativeNElements[midpoint] <= idxInCluster) { + left = midpoint + 1; + continue; } - firstInPage += pi.fNElements; - ++pageNo; + + if ((midpoint == 0) || (fCumulativeNElements[midpoint - 1] <= idxInCluster)) + break; + + right = midpoint - 1; } + R__ASSERT(midpoint < N); + + auto pageInfo = fPageInfos[midpoint]; + decltype(idxInCluster) firstInPage = (midpoint == 0) ? 0 : fCumulativeNElements[midpoint - 1]; R__ASSERT(firstInPage <= idxInCluster); R__ASSERT((firstInPage + pageInfo.fNElements) > idxInCluster); - return RPageInfoExtended{pageInfo, firstInPage, pageNo}; + return RPageInfoExtended{pageInfo, firstInPage, midpoint}; } std::size_t @@ -824,10 +835,17 @@ ROOT::Experimental::Internal::RClusterDescriptorBuilder::MoveDescriptor() return R__FAIL("unset cluster ID"); if (fCluster.fNEntries == 0) return R__FAIL("empty cluster"); - for (const auto &pr : fCluster.fPageRanges) { + for (auto &pr : fCluster.fPageRanges) { if (fCluster.fColumnRanges.count(pr.first) == 0) { return R__FAIL("missing column range"); } + pr.second.fCumulativeNElements.clear(); + pr.second.fCumulativeNElements.reserve(pr.second.fPageInfos.size()); + NTupleSize_t sum = 0; + for (const auto &pi : pr.second.fPageInfos) { + sum += pi.fNElements; + pr.second.fCumulativeNElements.emplace_back(sum); + } } RClusterDescriptor result; std::swap(result, fCluster); From f3c9d40cc6965817ae93b39b6e696f6b397a7ec0 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 21 Nov 2024 14:53:30 +0100 Subject: [PATCH 6/8] [ntuple] update and enable some limits tests --- tree/ntuple/v7/test/ntuple_limits.cxx | 36 +++++++++++++-------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/tree/ntuple/v7/test/ntuple_limits.cxx b/tree/ntuple/v7/test/ntuple_limits.cxx index b5c5cae25fc2a..d562b87f02b25 100644 --- a/tree/ntuple/v7/test/ntuple_limits.cxx +++ b/tree/ntuple/v7/test/ntuple_limits.cxx @@ -14,9 +14,9 @@ // ./tree/ntuple/v7/test/ntuple_limits --gtest_also_run_disabled_tests --gtest_filter=*Limits_ManyClusters // ``` -TEST(RNTuple, DISABLED_Limits_ManyFields) +TEST(RNTuple, Limits_ManyFields) { - // Writing and reading a model with 100k integer fields takes around 2s and seems to have more than linear + // Writing and reading a model with 100k integer fields takes around 2.2s and seems to have slightly more than linear // complexity (200k fields take 7.5s). FileRaii fileGuard("test_ntuple_limits_manyFields.root"); @@ -49,13 +49,13 @@ TEST(RNTuple, DISABLED_Limits_ManyFields) } } -TEST(RNTuple, DISABLED_Limits_ManyClusters) +TEST(RNTuple, Limits_ManyClusters) { - // Writing and reading 100k clusters takes between 80s - 100s and seems to have more than quadratic complexity - // (50k clusters take less than 15s). + // Writing and reading 500k clusters takes around 3.3s and seems to have benign scaling behavior. + // (1M clusters take around 6.6s). FileRaii fileGuard("test_ntuple_limits_manyClusters.root"); - static constexpr int NumClusters = 100'000; + static constexpr int NumClusters = 500'000; { auto model = RNTupleModel::Create(); @@ -84,13 +84,13 @@ TEST(RNTuple, DISABLED_Limits_ManyClusters) } } -TEST(RNTuple, DISABLED_Limits_ManyClusterGroups) +TEST(RNTuple, Limits_ManyClusterGroups) { - // Writing and reading 100k cluster groups takes between 100s - 110s and seems to have more than quadratic complexity - // (50k cluster groups takes less than 20s). + // Writing and reading 25k cluster groups takes around 1.7s and seems to have quadratic complexity + // (50k cluster groups takes around 6.5s). FileRaii fileGuard("test_ntuple_limits_manyClusterGroups.root"); - static constexpr int NumClusterGroups = 100'000; + static constexpr int NumClusterGroups = 25'000; { auto model = RNTupleModel::Create(); @@ -119,13 +119,13 @@ TEST(RNTuple, DISABLED_Limits_ManyClusterGroups) } } -TEST(RNTuple, DISABLED_Limits_ManyPages) +TEST(RNTuple, Limits_ManyPages) { - // Writing and reading 200k pages (of two elements each) takes around 13s and seems to have more than quadratic - // complexity (400k pages take 100s). + // Writing and reading 1M pages (of two elements each) takes around 1.3 and seems to have benign scaling behavior + // (2M pages take 2.6s). FileRaii fileGuard("test_ntuple_limits_manyPages.root"); - static constexpr int NumPages = 200'000; + static constexpr int NumPages = 1'000'000; static constexpr int NumEntries = NumPages * 2; { @@ -160,13 +160,13 @@ TEST(RNTuple, DISABLED_Limits_ManyPages) } } -TEST(RNTuple, DISABLED_Limits_ManyPagesOneEntry) +TEST(RNTuple, Limits_ManyPagesOneEntry) { - // Writing and reading 200k pages (of four elements each) takes around 13s and seems to have more than quadratic - // complexity (400k pages take around 100s). + // Writing and reading 1M pages (of four elements each) takes around 2.4s and seems to have benign scaling behavior + // (2M pages take around 4.8s). FileRaii fileGuard("test_ntuple_limits_manyPagesOneEntry.root"); - static constexpr int NumPages = 200'000; + static constexpr int NumPages = 1'000'000; static constexpr int NumElements = NumPages * 4; { From 5bd22055f5e08c73aee167433388c5b6c28a7cab Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 28 Nov 2024 15:57:50 +0100 Subject: [PATCH 7/8] [NFC][ntuple] fix typo in code comment --- tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx index 85cd22b6b5a4e..ff2cb565a3825 100644 --- a/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx @@ -577,7 +577,7 @@ private: std::vector fExtraTypeInfoDescriptors; std::unique_ptr fHeaderExtension; - // We don't expose this publicy because when we add sharded clusters, this interface does not make sense anymore + // We don't expose this publicly because when we add sharded clusters, this interface does not make sense anymore DescriptorId_t FindClusterId(NTupleSize_t entryIdx) const; public: From d5eae81e5e234167fbabae073a0473c1bc852ea9 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 12 Dec 2024 23:25:35 +0100 Subject: [PATCH 8/8] [NFC][ntuple] note down peak RSS of limits tests --- tree/ntuple/v7/test/ntuple_limits.cxx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tree/ntuple/v7/test/ntuple_limits.cxx b/tree/ntuple/v7/test/ntuple_limits.cxx index d562b87f02b25..a3d236e3a7d42 100644 --- a/tree/ntuple/v7/test/ntuple_limits.cxx +++ b/tree/ntuple/v7/test/ntuple_limits.cxx @@ -18,6 +18,7 @@ TEST(RNTuple, Limits_ManyFields) { // Writing and reading a model with 100k integer fields takes around 2.2s and seems to have slightly more than linear // complexity (200k fields take 7.5s). + // Peak RSS is around 750MB. FileRaii fileGuard("test_ntuple_limits_manyFields.root"); static constexpr int NumFields = 100'000; @@ -53,6 +54,7 @@ TEST(RNTuple, Limits_ManyClusters) { // Writing and reading 500k clusters takes around 3.3s and seems to have benign scaling behavior. // (1M clusters take around 6.6s). + // Peak RSS is around 850MB. FileRaii fileGuard("test_ntuple_limits_manyClusters.root"); static constexpr int NumClusters = 500'000; @@ -88,6 +90,7 @@ TEST(RNTuple, Limits_ManyClusterGroups) { // Writing and reading 25k cluster groups takes around 1.7s and seems to have quadratic complexity // (50k cluster groups takes around 6.5s). + // Peak RSS is around 275MB. FileRaii fileGuard("test_ntuple_limits_manyClusterGroups.root"); static constexpr int NumClusterGroups = 25'000; @@ -123,6 +126,7 @@ TEST(RNTuple, Limits_ManyPages) { // Writing and reading 1M pages (of two elements each) takes around 1.3 and seems to have benign scaling behavior // (2M pages take 2.6s). + // Peak RSS is around 600MB. FileRaii fileGuard("test_ntuple_limits_manyPages.root"); static constexpr int NumPages = 1'000'000; @@ -164,6 +168,7 @@ TEST(RNTuple, Limits_ManyPagesOneEntry) { // Writing and reading 1M pages (of four elements each) takes around 2.4s and seems to have benign scaling behavior // (2M pages take around 4.8s). + // Peak RSS is around 625MB. FileRaii fileGuard("test_ntuple_limits_manyPagesOneEntry.root"); static constexpr int NumPages = 1'000'000; @@ -207,6 +212,7 @@ TEST(RNTuple, DISABLED_Limits_LargePage) { // Writing and reading one page with 600M elements takes around 18s and seems to have linear complexity // (900M elements take 27s) + // Peak RSS is around 14 GB. FileRaii fileGuard("test_ntuple_limits_largePage.root"); // clang-format off @@ -255,6 +261,7 @@ TEST(RNTuple, DISABLED_Limits_LargePageOneEntry) { // Writing and reading one page with 100M elements takes around 1.7s and seems to have linear complexity (200M // elements take 3.5s, 400M elements take around 7s). + // Peak RSS is around 1.4GB. FileRaii fileGuard("test_ntuple_limits_largePageOneEntry.root"); static constexpr int NumElements = 100'000'000;