From bcc41abab07d0704dfb03f82c9503562e972d021 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 26 Sep 2026 22:41:32 +0200 Subject: [PATCH 01/24] [ntuple] fix mock page source in cluster unit test --- tree/ntuple/test/ntuple_cluster.cxx | 75 ++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/tree/ntuple/test/ntuple_cluster.cxx b/tree/ntuple/test/ntuple_cluster.cxx index 6121a09d61e1e..30b223f384186 100644 --- a/tree/ntuple/test/ntuple_cluster.cxx +++ b/tree/ntuple/test/ntuple_cluster.cxx @@ -37,29 +37,27 @@ namespace { * Used to track LoadClusters calls triggered by ClusterPool::GetCluster */ class RPageSourceMock : public RPageSource { + std::uint32_t fSzPageList = 0; + std::unique_ptr fBufPageList; + protected: void LoadStructureImpl() final {} - RNTupleDescriptor AttachImpl() final { return RNTupleDescriptor(); } - std::unique_ptr CloneImpl() const final { return nullptr; } - void LoadPageListImpl(const ROOT::RNTupleLocator &, unsigned char *) final {} - void LoadSealedPageImpl(const ROOT::RNTupleLocator &, RSealedPage &) final {} - std::unique_ptr - OpenWithDifferentAnchor(const ROOT::Internal::RNTupleLink &, const ROOT::RNTupleReadOptions &) final - { - throw ROOT::RException(R__FAIL("method not implemented")); - } - -public: - /// Records the cluster IDs requests by LoadClusters() calls - std::vector fReqsClusterIds; - std::vector fReqsColumns; - - RPageSourceMock() : RPageSource("test", RNTupleReadOptions()) + RNTupleDescriptor AttachImpl() final { ROOT::Internal::RNTupleDescriptorBuilder descBuilder; // Should set the version from the anchor; here it is fine to use the same version as for writing. descBuilder.SetVersionForWriting(); descBuilder.SetNTuple("ntpl", ""); + descBuilder.AddField(ROOT::Internal::RFieldDescriptorBuilder() + .FieldId(0) + .FieldName("") + .Structure(ROOT::ENTupleStructure::kRecord) + .MoveDescriptor() + .Unwrap()); + + auto ctx = RNTupleSerializer::SerializeHeader(nullptr, descBuilder.GetDescriptor()).Unwrap(); + + std::vector physClusterIDs; for (unsigned i = 0; i <= 5; ++i) { descBuilder.AddCluster(ROOT::Internal::RClusterDescriptorBuilder() .ClusterId(i) @@ -67,14 +65,45 @@ class RPageSourceMock : public RPageSource { .NEntries(1) .MoveDescriptor() .Unwrap()); + physClusterIDs.emplace_back(ctx.MapClusterId(i)); } + + fSzPageList = + RNTupleSerializer::SerializePageList(nullptr, descBuilder.GetDescriptor(), physClusterIDs, ctx).Unwrap(); + fBufPageList = MakeUninitArray(fSzPageList); + RNTupleSerializer::SerializePageList(fBufPageList.get(), descBuilder.GetDescriptor(), physClusterIDs, ctx); + RNTupleLocator locator; + locator.SetNBytesOnStorage(fSzPageList); + ROOT::Internal::RClusterGroupDescriptorBuilder cgBuilder; + cgBuilder.PageListLocator(locator).PageListLength(fSzPageList); 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()); + + // We need to return a descriptor without clusters + auto desc = descBuilder.MoveDescriptor(); + desc.DropClusterGroupDetails(0); + return desc; + } + std::unique_ptr CloneImpl() const final { return nullptr; } + void LoadPageListImpl(const ROOT::RNTupleLocator &, unsigned char *buf) final + { + memcpy(buf, fBufPageList.get(), fSzPageList); } + void LoadSealedPageImpl(const ROOT::RNTupleLocator &, RSealedPage &) final {} + std::unique_ptr + OpenWithDifferentAnchor(const ROOT::Internal::RNTupleLink &, const ROOT::RNTupleReadOptions &) final + { + throw ROOT::RException(R__FAIL("method not implemented")); + } + +public: + /// Records the cluster IDs requests by LoadClusters() calls + std::vector fReqsClusterIds; + std::vector fReqsColumns; + + RPageSourceMock() : RPageSource("test", RNTupleReadOptions()) {} std::vector> LoadClusters(std::span clusterKeys) final { std::vector> result; @@ -202,6 +231,7 @@ TEST(Cluster, AdoptClusters) TEST(ClusterPool, GetClusterBasics) { RPageSourceMock p1; + p1.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); RClusterPool c1(p1, 1); c1.GetCluster(3, {0}); c1.WaitForInFlightClusters(); @@ -212,6 +242,7 @@ TEST(ClusterPool, GetClusterBasics) EXPECT_EQ(RCluster::ColumnSet_t({0}), p1.fReqsColumns[1]); RPageSourceMock p2; + p2.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); { RClusterPool c2(p2, 2); c2.GetCluster(0, {0}); @@ -228,6 +259,7 @@ TEST(ClusterPool, GetClusterBasics) EXPECT_EQ(RCluster::ColumnSet_t({0}), p2.fReqsColumns[3]); RPageSourceMock p3; + p3.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); { RClusterPool c3(p3, 2); c3.GetCluster(0, {0}); @@ -241,6 +273,7 @@ TEST(ClusterPool, GetClusterBasics) EXPECT_EQ(3U, p3.fReqsClusterIds[3]); RPageSourceMock p4; + p4.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); { RClusterPool c4(p4, 3); c4.GetCluster(2, {0}); @@ -256,6 +289,7 @@ TEST(ClusterPool, GetClusterBasics) TEST(ClusterPool, SetEntryRange) { RPageSourceMock p1; + p1.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); p1.SetEntryRange({0, 6}); RClusterPool c1(p1, 1); c1.GetCluster(3, {0}); @@ -265,6 +299,7 @@ TEST(ClusterPool, SetEntryRange) EXPECT_EQ(4U, p1.fReqsClusterIds[1]); RPageSourceMock p2; + p2.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); p2.SetEntryRange({3, 1}); RClusterPool c2(p2, 1); c2.GetCluster(3, {0}); @@ -273,6 +308,7 @@ TEST(ClusterPool, SetEntryRange) EXPECT_EQ(3U, p2.fReqsClusterIds[0]); RPageSourceMock p3; + p3.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); p3.SetEntryRange({0, 1}); RClusterPool c3(p3, 1); c3.GetCluster(3, {0}); @@ -281,6 +317,7 @@ TEST(ClusterPool, SetEntryRange) EXPECT_EQ(3U, p3.fReqsClusterIds[0]); RPageSourceMock p4; + p4.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); p4.SetEntryRange({0, 3}); RClusterPool c4(p4, 2); c4.GetCluster(0, {0}); @@ -294,6 +331,7 @@ TEST(ClusterPool, SetEntryRange) TEST(ClusterPool, GetClusterIncrementally) { RPageSourceMock p1; + p1.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); RClusterPool c1(p1, 1); c1.GetCluster(3, {0}); c1.WaitForInFlightClusters(); @@ -311,6 +349,7 @@ TEST(ClusterPool, GetClusterIncrementally) TEST(ClusterPool, PinCluster) { RPageSourceMock p1; + p1.Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); RClusterPool c1(p1, 1); p1.PinCluster(3); c1.GetCluster(3, {0}); From c8984e13f4214ad55c364bce54903d472cc4360a Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sun, 27 Sep 2026 22:16:09 +0200 Subject: [PATCH 02/24] [ntuple] fix descriptor locking in join table's entry mapping --- tree/ntuple/src/RNTupleJoinTable.cxx | 42 +++++++++++++++------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/tree/ntuple/src/RNTupleJoinTable.cxx b/tree/ntuple/src/RNTupleJoinTable.cxx index eacfa45d76578..d531266893455 100644 --- a/tree/ntuple/src/RNTupleJoinTable.cxx +++ b/tree/ntuple/src/RNTupleJoinTable.cxx @@ -24,32 +24,36 @@ ROOT::Experimental::Internal::RNTupleJoinTable::REntryMapping::REntryMapping( "std::uint32_t", "std::uint64_t"}; pageSource.Attach(); - auto desc = pageSource.GetSharedDescriptorGuard(); - std::vector> fields; std::vector fieldValues; fieldValues.reserve(fJoinFieldNames.size()); + { + auto descGuard = pageSource.GetSharedDescriptorGuard(); + + for (const auto &fieldName : fJoinFieldNames) { + auto fieldId = descGuard->FindFieldId(fieldName); + if (fieldId == ROOT::kInvalidDescriptorId) { + throw RException(R__FAIL("could not find join field \"" + std::string(fieldName) + "\" in RNTuple \"" + + descGuard->GetName() + "\"")); + } - for (const auto &fieldName : fJoinFieldNames) { - auto fieldId = desc->FindFieldId(fieldName); - if (fieldId == ROOT::kInvalidDescriptorId) - throw RException(R__FAIL("could not find join field \"" + std::string(fieldName) + "\" in RNTuple \"" + - pageSource.GetNTupleName() + "\"")); - - const auto &fieldDesc = desc->GetFieldDescriptor(fieldId); + const auto &fieldDesc = descGuard->GetFieldDescriptor(fieldId); - if (allowedTypes.find(fieldDesc.GetTypeName()) == allowedTypes.end()) { - throw RException(R__FAIL("cannot use field \"" + fieldName + "\" with type \"" + fieldDesc.GetTypeName() + - "\" in join table: only integral types are allowed")); - } + if (allowedTypes.find(fieldDesc.GetTypeName()) == allowedTypes.end()) { + throw RException(R__FAIL("cannot use field \"" + fieldName + "\" with type \"" + fieldDesc.GetTypeName() + + "\" in join table: only integral types are allowed")); + } - auto field = std::make_unique>(fieldDesc.GetFieldName()); - field->SetOnDiskId(fieldDesc.GetId()); - ROOT::Internal::CallConnectPageSourceOnField(*field, pageSource); + auto field = std::make_unique>(fieldDesc.GetFieldName()); + field->SetOnDiskId(fieldDesc.GetId()); - fieldValues.emplace_back(field->CreateValue()); - fJoinFieldValueSizes.emplace_back(field->GetValueSize()); - fields.emplace_back(std::move(field)); + fieldValues.emplace_back(field->CreateValue()); + fJoinFieldValueSizes.emplace_back(field->GetValueSize()); + fields.emplace_back(std::move(field)); + } + } // descGuard + for (auto &f : fields) { + ROOT::Internal::CallConnectPageSourceOnField(*f, pageSource); } std::vector castJoinValues; From 9740ae2bbebc1d65a3ad78964b8992b799714e00 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 28 Sep 2026 00:02:08 +0200 Subject: [PATCH 03/24] [ntuple] fix descriptor locking in merger unit test --- tree/ntuple/test/ntuple_merger.cxx | 3 --- 1 file changed, 3 deletions(-) diff --git a/tree/ntuple/test/ntuple_merger.cxx b/tree/ntuple/test/ntuple_merger.cxx index 76912f77d9346..73267ea9028da 100644 --- a/tree/ntuple/test/ntuple_merger.cxx +++ b/tree/ntuple/test/ntuple_merger.cxx @@ -837,9 +837,6 @@ static bool VerifyPageCompression(const std::string_view fileName, std::uint32_t // Check that the actual compression is correct auto source = RPageSource::Create("ntuple", fileName); source->Attach(); - auto descriptor = source->GetSharedDescriptorGuard(); - const auto &columnDesc = descriptor->GetColumnDescriptor(0); - const auto colElement = ROOT::Internal::RColumnElementBase::Generate(columnDesc.GetType()); RPageStorage::RSealedPage sealedPage; source->LoadSealedPage(0, {0, 0}, sealedPage); auto buffer = MakeUninitArray(sealedPage.GetBufferSize()); From 58cc690b619aeb9d73314be0969a2d4fd355c1fc Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 28 Sep 2026 09:43:23 +0200 Subject: [PATCH 04/24] [ntuple] minor fix in RClusterPool --- tree/ntuple/src/RClusterPool.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/ntuple/src/RClusterPool.cxx b/tree/ntuple/src/RClusterPool.cxx index 4f64cd14a0e33..3b9a38f68e779 100644 --- a/tree/ntuple/src/RClusterPool.cxx +++ b/tree/ntuple/src/RClusterPool.cxx @@ -205,7 +205,7 @@ ROOT::Internal::RClusterPool::GetCluster(ROOT::DescriptorId_t clusterId, const R auto cid = next; auto descriptorGuard = fPageSource.FindNextClusterId(cid, next); - if (next != ROOT::kInvalidNTupleIndex) { + if (next != ROOT::kInvalidDescriptorId) { if (!fPageSource.GetEntryRange().IntersectsWith(descriptorGuard->GetClusterDescriptor(next))) next = ROOT::kInvalidDescriptorId; } From e7897f58b8efceb3326243da4494d448257c4c66 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 28 Sep 2026 13:44:23 +0200 Subject: [PATCH 05/24] [ntuple] add private RPageSource::LoadPageList() Factor out loading a page list (and thus populating cluster group details) into a separate method, to be reused later. --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 7 ++++++ tree/ntuple/src/RPageStorage.cxx | 32 ++++++++++++++++++--------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index a205bba72155b..152eb1e3daa16 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -696,6 +696,10 @@ private: bool fIsAttached = false; ///< Set to true once `Attach()` is called bool fHasStreamerInfosRegistered = false; ///< Set to true when RegisterStreamerInfos() is called. + /// The interpretation of page lists, set in Attach() + Internal::RNTupleSerializer::EDescriptorDeserializeMode fDeserializationMode = + Internal::RNTupleSerializer::EDescriptorDeserializeMode::kRaw; + /// The active columns are implicitly defined by the model fields or views RActivePhysicalColumns fActivePhysicalColumns; @@ -724,6 +728,9 @@ private: /// Must not be called when the descriptor guard is taken. void UpdateLastUsedCluster(ROOT::DescriptorId_t clusterId); + // Populate the cluster details of the given cluster group. Must hold an exclusive descriptor lock when calling. + // No-op if the cluster details are already present. + void LoadPageList(DescriptorId_t clusterGroupId, const RExclDescriptorGuard &exclGuard); // Common treatment of zero pages in LoadPageFromSummary() ROOT::Internal::RPageRef LoadZeroPage(ColumnHandle_t columnHandle, const RPageSummary &pageSummary); // Once the page is found to be missing in the page cache and all information about the page is collected, diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index ef2aba2f034ab..311a093a8f8e5 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -238,27 +238,38 @@ void ROOT::Internal::RPageSource::Attach(RNTupleSerializer::EDescriptorDeseriali if (fIsAttached) return; + fDeserializationMode = mode; + LoadStructure(); auto descGuard = GetExclDescriptorGuard(); descGuard.MoveIn(AttachImpl()); fStructureBuffer.Reset(); - std::vector buffer; - for (const auto &cgDesc : descGuard->GetClusterGroupIterable()) { - buffer.resize(cgDesc.GetPageListLength() + cgDesc.GetPageListLocator().GetNBytesOnStorage()); - auto zipBuffer = buffer.data() + cgDesc.GetPageListLength(); - - LoadPageListImpl(cgDesc.GetPageListLocator(), zipBuffer); - RNTupleDecompressor::Unzip(zipBuffer, cgDesc.GetPageListLocator().GetNBytesOnStorage(), - cgDesc.GetPageListLength(), buffer.data()); - RNTupleSerializer::DeserializePageList(buffer.data(), cgDesc.GetPageListLength(), cgDesc.GetId(), *descGuard, - mode); + // For a descriptor coming from disk, we know that cluster group IDs are issued consecutively + for (DescriptorId_t cgId = 0; cgId < descGuard->GetNClusterGroups(); ++cgId) { + LoadPageList(cgId, descGuard); } fIsAttached = true; } +void ROOT::Internal::RPageSource::LoadPageList(DescriptorId_t clusterGroupId, const RExclDescriptorGuard &exclGuard) +{ + const auto &cgDesc = exclGuard->GetClusterGroupDescriptor(clusterGroupId); + if (cgDesc.HasClusterDetails()) + return; + + auto buffer = + MakeUninitArray(cgDesc.GetPageListLength() + cgDesc.GetPageListLocator().GetNBytesOnStorage()); + auto zipBuffer = buffer.get() + cgDesc.GetPageListLength(); + LoadPageListImpl(cgDesc.GetPageListLocator(), zipBuffer); + RNTupleDecompressor::Unzip(zipBuffer, cgDesc.GetPageListLocator().GetNBytesOnStorage(), cgDesc.GetPageListLength(), + buffer.get()); + RNTupleSerializer::DeserializePageList(buffer.get(), cgDesc.GetPageListLength(), cgDesc.GetId(), *exclGuard, + fDeserializationMode); +} + std::unique_ptr ROOT::Internal::RPageSource::Clone() const { auto clone = CloneImpl(); @@ -266,6 +277,7 @@ std::unique_ptr ROOT::Internal::RPageSource::Clone( clone->GetExclDescriptorGuard().MoveIn(GetSharedDescriptorGuard()->Clone()); clone->fHasStructure = true; clone->fIsAttached = true; + clone->fDeserializationMode = fDeserializationMode; } return clone; } From bb6efb000757a0af02ce9ba559fe6a40e9b3f254 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 25 Sep 2026 13:19:36 +0200 Subject: [PATCH 06/24] [ntuple] allow for manual release of RSharedDescriptorGuard --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 152eb1e3daa16..9a238bf1e17bb 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -628,9 +628,20 @@ public: RSharedDescriptorGuard &operator=(const RSharedDescriptorGuard &) = delete; RSharedDescriptorGuard(RSharedDescriptorGuard &&) = default; RSharedDescriptorGuard &operator=(RSharedDescriptorGuard &&) = default; - ~RSharedDescriptorGuard() { fLock->unlock_shared(); } + ~RSharedDescriptorGuard() + { + if (IsValid()) + Release(); + } const ROOT::RNTupleDescriptor *operator->() const { return fDescriptor; } const ROOT::RNTupleDescriptor &GetRef() const { return *fDescriptor; } + bool IsValid() const { return fLock; } + void Release() + { + fLock->unlock_shared(); + fLock = nullptr; + fDescriptor = nullptr; + } }; /// An RAII wrapper used for the writable access to `RPageSource::fDescriptor`. See `GetSharedDescriptorGuard()`. From 14dbd6cf3a71bc74170deb4a04594e3fe7713e94 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 25 Sep 2026 13:26:40 +0200 Subject: [PATCH 07/24] [ntuple] make R[Shared|Excl]DescriptorGuard default constructible --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 9a238bf1e17bb..2c456d390285c 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -619,6 +619,7 @@ public: std::shared_mutex *fLock = nullptr; public: + RSharedDescriptorGuard() = default; RSharedDescriptorGuard(const ROOT::RNTupleDescriptor &desc, std::shared_mutex &lock) : fDescriptor(&desc), fLock(&lock) { @@ -646,13 +647,14 @@ public: /// An RAII wrapper used for the writable access to `RPageSource::fDescriptor`. See `GetSharedDescriptorGuard()`. class RExclDescriptorGuard { - ROOT::RNTupleDescriptor &fDescriptor; - std::shared_mutex &fLock; + ROOT::RNTupleDescriptor *fDescriptor = nullptr; + std::shared_mutex *fLock = nullptr; public: - RExclDescriptorGuard(ROOT::RNTupleDescriptor &desc, std::shared_mutex &lock) : fDescriptor(desc), fLock(lock) + RExclDescriptorGuard() = default; + RExclDescriptorGuard(ROOT::RNTupleDescriptor &desc, std::shared_mutex &lock) : fDescriptor(&desc), fLock(&lock) { - fLock.lock(); + fLock->lock(); } RExclDescriptorGuard(const RExclDescriptorGuard &) = delete; RExclDescriptorGuard &operator=(const RExclDescriptorGuard &) = delete; @@ -660,12 +662,14 @@ public: RExclDescriptorGuard &operator=(RExclDescriptorGuard &&) = delete; ~RExclDescriptorGuard() { - fDescriptor.IncGeneration(); - fLock.unlock(); + if (fLock) { + fDescriptor->IncGeneration(); + fLock->unlock(); + } } - ROOT::RNTupleDescriptor &operator*() const { return fDescriptor; } - ROOT::RNTupleDescriptor *operator->() const { return &fDescriptor; } - void MoveIn(ROOT::RNTupleDescriptor desc) { fDescriptor = std::move(desc); } + ROOT::RNTupleDescriptor &operator*() const { return *fDescriptor; } + ROOT::RNTupleDescriptor *operator->() const { return fDescriptor; } + void MoveIn(ROOT::RNTupleDescriptor desc) { *fDescriptor = std::move(desc); } }; private: From 7b1a25865b5e67031ea627e04653efcd3d8d18c5 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 25 Sep 2026 15:44:18 +0200 Subject: [PATCH 08/24] [ntuple] fix moving RSharedDescriptorGuard --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 2c456d390285c..ef8a5f17bba15 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -627,8 +627,19 @@ public: } RSharedDescriptorGuard(const RSharedDescriptorGuard &) = delete; RSharedDescriptorGuard &operator=(const RSharedDescriptorGuard &) = delete; - RSharedDescriptorGuard(RSharedDescriptorGuard &&) = default; - RSharedDescriptorGuard &operator=(RSharedDescriptorGuard &&) = default; + RSharedDescriptorGuard(RSharedDescriptorGuard &&other) + { + std::swap(fDescriptor, other.fDescriptor); + std::swap(fLock, other.fLock); + } + RSharedDescriptorGuard &operator=(RSharedDescriptorGuard &&other) + { + if (this == &other) + return *this; + std::swap(fDescriptor, other.fDescriptor); + std::swap(fLock, other.fLock); + return *this; + } ~RSharedDescriptorGuard() { if (IsValid()) From 52d67c4f7aa863643eb490646cb1ed6212ac4e72 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 25 Sep 2026 13:45:58 +0200 Subject: [PATCH 09/24] [ntuple] make RExclDescriptorGuard move constructible --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index ef8a5f17bba15..1b1a6298fb9a1 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -669,8 +669,19 @@ public: } RExclDescriptorGuard(const RExclDescriptorGuard &) = delete; RExclDescriptorGuard &operator=(const RExclDescriptorGuard &) = delete; - RExclDescriptorGuard(RExclDescriptorGuard &&) = delete; - RExclDescriptorGuard &operator=(RExclDescriptorGuard &&) = delete; + RExclDescriptorGuard(RExclDescriptorGuard &&other) + { + std::swap(fDescriptor, other.fDescriptor); + std::swap(fLock, other.fLock); + } + RExclDescriptorGuard &operator=(RExclDescriptorGuard &&other) + { + if (this == &other) + return *this; + std::swap(fDescriptor, other.fDescriptor); + std::swap(fLock, other.fLock); + return *this; + } ~RExclDescriptorGuard() { if (fLock) { From 42c8cab66b2ae468a1f1f7646102ba510968a998 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 26 Sep 2026 23:24:38 +0200 Subject: [PATCH 10/24] [ntuple] allow for manual release of RExclDescriptorGuard --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 1b1a6298fb9a1..0d650f8dbb6de 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -684,14 +684,20 @@ public: } ~RExclDescriptorGuard() { - if (fLock) { - fDescriptor->IncGeneration(); - fLock->unlock(); - } + if (IsValid()) + Release(); } ROOT::RNTupleDescriptor &operator*() const { return *fDescriptor; } ROOT::RNTupleDescriptor *operator->() const { return fDescriptor; } void MoveIn(ROOT::RNTupleDescriptor desc) { *fDescriptor = std::move(desc); } + bool IsValid() const { return fLock; } + void Release() + { + fDescriptor->IncGeneration(); + fLock->unlock(); + fLock = nullptr; + fDescriptor = nullptr; + } }; private: From 1a424917e1ab38128162b7042ec29d30fbb3994f Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 25 Sep 2026 13:27:21 +0200 Subject: [PATCH 11/24] [ntuple] add RPageSource::RAnyDescriptorGuard --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 0d650f8dbb6de..cf64d0166ac7e 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -40,6 +40,7 @@ #include #include #include +#include #include namespace ROOT { @@ -700,6 +701,41 @@ public: } }; + /// Either a shared or an exclusive lock guard + class RAnyDescriptorGuard { + private: + std::variant fAnyGuard; + + public: + RAnyDescriptorGuard(RSharedDescriptorGuard sharedGuard) : fAnyGuard(std::move(sharedGuard)) {} + RAnyDescriptorGuard(RExclDescriptorGuard exclGuard) : fAnyGuard(std::move(exclGuard)) {} + RAnyDescriptorGuard(const RAnyDescriptorGuard &) = delete; + RAnyDescriptorGuard &operator=(const RAnyDescriptorGuard &) = delete; + RAnyDescriptorGuard(RAnyDescriptorGuard &&) = default; + RAnyDescriptorGuard &operator=(RAnyDescriptorGuard &&) = default; + ~RAnyDescriptorGuard() = default; + + const ROOT::RNTupleDescriptor &GetRef() const + { + return (fAnyGuard.index() == 0) ? std::get<0>(fAnyGuard).GetRef() : std::get<1>(fAnyGuard).operator*(); + } + const ROOT::RNTupleDescriptor *operator->() const + { + return (fAnyGuard.index() == 0) ? std::get<0>(fAnyGuard).operator->() : std::get<1>(fAnyGuard).operator->(); + } + bool IsValid() const + { + return (fAnyGuard.index() == 0) ? std::get<0>(fAnyGuard).IsValid() : std::get<1>(fAnyGuard).IsValid(); + } + void Release() + { + if (fAnyGuard.index() == 0) + std::get<0>(fAnyGuard).Release(); + else + std::get<1>(fAnyGuard).Release(); + } + }; + private: /// Keeps track of the requested physical column IDs and their in-memory target type via a column element identifier. /// When using alias columns (projected fields), physical columns may be requested multiple times. From fae152c8abd3b39b7a29f75b61c65d93fa35fcce Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 25 Sep 2026 14:34:59 +0200 Subject: [PATCH 12/24] [ntuple] add private RPageSource::EnsureClusterDetails() --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 5 +++++ tree/ntuple/src/RPageStorage.cxx | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index cf64d0166ac7e..7e37c5b1ede6e 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -802,6 +802,11 @@ private: /// Pages of pinned clusters won't be evicted from the page pool. std::unordered_set fPinnedClusters; + /// 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 + /// passed one or a new, exclusive guard if cluster details needed to be loaded. + RAnyDescriptorGuard EnsureClusterDetails(DescriptorId_t cgId, RAnyDescriptorGuard descGuard); + /// Does nothing if fLastUsedCluster == clusterId. Otherwise, updated fLastUsedCluster /// and evict unused paged from the page pool of all previous clusters. /// Must not be called when the descriptor guard is taken. diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 311a093a8f8e5..9694c680bb550 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -282,6 +282,28 @@ std::unique_ptr ROOT::Internal::RPageSource::Clone( return clone; } +ROOT::Internal::RPageSource::RAnyDescriptorGuard +ROOT::Internal::RPageSource::EnsureClusterDetails(DescriptorId_t cgId, RAnyDescriptorGuard descGuard) +{ + assert(descGuard.IsValid()); + if ((cgId == kInvalidDescriptorId) || descGuard->GetClusterGroupDescriptor(cgId).HasClusterDetails()) + return descGuard; + + descGuard.Release(); + + auto exclGuard = GetExclDescriptorGuard(); + + if (exclGuard->GetClusterGroupDescriptor(cgId).HasClusterDetails()) { + // unlikely, but between releasing the shared guard and acquiring the exclusive guard, the cluster details + // may have appeared + return exclGuard; + } + + LoadPageList(cgId, exclGuard); + + return exclGuard; +} + ROOT::NTupleSize_t ROOT::Internal::RPageSource::GetNEntries() { return GetSharedDescriptorGuard()->GetNEntries(); From be7fa9289832f2fcc0845cdd63e2cc213e2c1c63 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 25 Sep 2026 11:37:30 +0200 Subject: [PATCH 13/24] [ntuple] add RPageStorage::FindClusterGroupId() --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 9 +++++++++ tree/ntuple/src/RPageStorage.cxx | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 7e37c5b1ede6e..e7dd37e12ec01 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -779,6 +779,13 @@ private: Internal::RNTupleSerializer::EDescriptorDeserializeMode fDeserializationMode = Internal::RNTupleSerializer::EDescriptorDeserializeMode::kRaw; + /// This vector is aligned with the cluster groups in the descriptor. It stores the cumulative number of clusters + /// in the cluster groups. Given a cluster ID, we can thus quickly determine the cluster group that the cluster + /// comes from. While descriptor IDs in a descriptor in general are arbitrary, for the descriptor in the page source, + /// that was created from a serialized on-disk representation, we know that cluster and cluster group IDs are + /// issued consecutively. + std::vector fCumulativeClusterCounts; + /// The active columns are implicitly defined by the model fields or views RActivePhysicalColumns fActivePhysicalColumns; @@ -806,6 +813,8 @@ private: /// to be called while holding the lock passed by descGuard. The returned descriptor guard is either the /// passed one or a new, exclusive guard if cluster details needed to be loaded. RAnyDescriptorGuard EnsureClusterDetails(DescriptorId_t cgId, RAnyDescriptorGuard descGuard); + /// Uses binary search in fCumulativeClusterCounts to determine the cluster group that the cluster ID belongs in. + DescriptorId_t FindClusterGroupId(DescriptorId_t clusterId) const; /// Does nothing if fLastUsedCluster == clusterId. Otherwise, updated fLastUsedCluster /// and evict unused paged from the page pool of all previous clusters. diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 9694c680bb550..3b6f114e2bc43 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -246,6 +246,13 @@ void ROOT::Internal::RPageSource::Attach(RNTupleSerializer::EDescriptorDeseriali descGuard.MoveIn(AttachImpl()); fStructureBuffer.Reset(); + NTupleSize_t sumClusterCount = 0; + fCumulativeClusterCounts.reserve(descGuard->GetNClusterGroups()); + for (const auto &cgDesc : descGuard->GetClusterGroupIterable()) { + sumClusterCount += cgDesc.GetNClusters(); + fCumulativeClusterCounts.emplace_back(sumClusterCount); + } + // For a descriptor coming from disk, we know that cluster group IDs are issued consecutively for (DescriptorId_t cgId = 0; cgId < descGuard->GetNClusterGroups(); ++cgId) { LoadPageList(cgId, descGuard); @@ -278,6 +285,7 @@ std::unique_ptr ROOT::Internal::RPageSource::Clone( clone->fHasStructure = true; clone->fIsAttached = true; clone->fDeserializationMode = fDeserializationMode; + clone->fCumulativeClusterCounts = fCumulativeClusterCounts; } return clone; } @@ -304,6 +312,16 @@ ROOT::Internal::RPageSource::EnsureClusterDetails(DescriptorId_t cgId, RAnyDescr return exclGuard; } +ROOT::DescriptorId_t ROOT::Internal::RPageSource::FindClusterGroupId(DescriptorId_t clusterId) const +{ + // We use `cluster + 1` because fCumulativeClusterCounts stores the cumulative _number_ of clusters and not + // the last cluster IDs. + auto iter = std::lower_bound(fCumulativeClusterCounts.begin(), fCumulativeClusterCounts.end(), clusterId + 1); + if (iter == fCumulativeClusterCounts.end()) + return kInvalidDescriptorId; + return std::distance(fCumulativeClusterCounts.begin(), iter); +} + ROOT::NTupleSize_t ROOT::Internal::RPageSource::GetNEntries() { return GetSharedDescriptorGuard()->GetNEntries(); From e07be3a77822b8a1de6a6f0000719a71d70f8cfd Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 28 Sep 2026 13:23:48 +0200 Subject: [PATCH 14/24] [ntuple] prepare for loading cluster details on demand ... by ensuring cluster details are available when cluster descriptors are requested. --- tree/ntuple/src/RPageStorage.cxx | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 3b6f114e2bc43..cc8973e27ab0a 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -329,15 +329,17 @@ ROOT::NTupleSize_t ROOT::Internal::RPageSource::GetNEntries() ROOT::NTupleSize_t ROOT::Internal::RPageSource::GetNElements(ROOT::DescriptorId_t physicalColumnId) { - auto descGuard = GetSharedDescriptorGuard(); - if (descGuard->GetNClusters() == 0) + auto sharedGuard = GetSharedDescriptorGuard(); + if (sharedGuard->GetNClusters() == 0) return 0; - auto itr = descGuard->GetClusterGroupIterable().begin(); - itr += descGuard->GetNClusterGroups() - 1; - R__ASSERT(itr->HasClusterDetails()); - const auto &cd = descGuard->GetClusterDescriptor(itr->GetClusterIds().back()); + const auto lastClusterGroupId = sharedGuard->GetNClusterGroups() - 1; + auto descGuard = EnsureClusterDetails(lastClusterGroupId, std::move(sharedGuard)); + + const auto &cgDesc = descGuard->GetClusterGroupDescriptor(lastClusterGroupId); + const auto &cd = descGuard->GetClusterDescriptor(cgDesc.GetClusterIds().back()); R__ASSERT(cd.ContainsColumn(physicalColumnId)); + const auto &columnRange = cd.GetColumnRange(physicalColumnId); return columnRange.GetFirstElementIndex() + columnRange.GetNElements(); } @@ -491,7 +493,7 @@ void ROOT::Internal::RPageSource::UnzipCluster(RCluster *cluster) RNTupleAtomicTimer timer(fCounters->fTimeWallUnzip, fCounters->fTimeCpuUnzip); const auto clusterId = cluster->GetId(); - auto descriptorGuard = GetSharedDescriptorGuard(); + auto descriptorGuard = EnsureClusterDetails(FindClusterGroupId(clusterId), GetSharedDescriptorGuard()); const auto &clusterDescriptor = descriptorGuard->GetClusterDescriptor(clusterId); fPreloadedClusters[clusterDescriptor.GetFirstEntryIndex()] = clusterId; @@ -586,7 +588,7 @@ void ROOT::Internal::RPageSource::PrepareLoadCluster( const std::function &perPageFunc) { - auto descriptorGuard = GetSharedDescriptorGuard(); + auto descriptorGuard = EnsureClusterDetails(FindClusterGroupId(clusterKey.fClusterId), GetSharedDescriptorGuard()); const auto &clusterDesc = descriptorGuard->GetClusterDescriptor(clusterKey.fClusterId); for (auto physicalColumnId : clusterKey.fPhysicalColumnSet) { @@ -613,8 +615,11 @@ void ROOT::Internal::RPageSource::UpdateLastUsedCluster(ROOT::DescriptorId_t clu if (fLastUsedCluster == clusterId) return; - ROOT::NTupleSize_t firstEntryIndex = - GetSharedDescriptorGuard()->GetClusterDescriptor(clusterId).GetFirstEntryIndex(); + ROOT::NTupleSize_t firstEntryIndex = kInvalidNTupleIndex; + { + auto descGuard = EnsureClusterDetails(FindClusterGroupId(clusterId), GetSharedDescriptorGuard()); + firstEntryIndex = descGuard->GetClusterDescriptor(clusterId).GetFirstEntryIndex(); + } auto itr = fPreloadedClusters.begin(); while ((itr != fPreloadedClusters.end()) && (itr->first < firstEntryIndex)) { if (fPinnedClusters.count(itr->second) > 0) { @@ -649,7 +654,7 @@ void ROOT::Internal::RPageSource::LoadSealedPage(ROOT::DescriptorId_t physicalCo ROOT::RClusterDescriptor::RPageInfo pageInfo; { - auto descriptorGuard = GetSharedDescriptorGuard(); + auto descriptorGuard = EnsureClusterDetails(FindClusterGroupId(clusterId), GetSharedDescriptorGuard()); const auto &clusterDescriptor = descriptorGuard->GetClusterDescriptor(clusterId); pageInfo = clusterDescriptor.GetPageRange(physicalColumnId).Find(localIndex.GetIndexInCluster()); } @@ -799,7 +804,7 @@ ROOT::Internal::RPageSource::LoadPage(ColumnHandle_t columnHandle, RNTupleLocalI RPageSummary pageSummary; { - auto descriptorGuard = GetSharedDescriptorGuard(); + auto descriptorGuard = EnsureClusterDetails(FindClusterGroupId(clusterId), GetSharedDescriptorGuard()); const auto &clusterDescriptor = descriptorGuard->GetClusterDescriptor(clusterId); const auto &columnRange = clusterDescriptor.GetColumnRange(columnId); if (columnRange.IsSuppressed()) From 8343ad2916d8996ed63a05eb7205925e0085a27b Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 26 Sep 2026 14:23:31 +0200 Subject: [PATCH 15/24] [ntuple] properly lock RPageSource::FindNextClusterId() --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 2 +- tree/ntuple/src/RPageStorage.cxx | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index e7dd37e12ec01..e1ca7fe6726cd 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -968,7 +968,7 @@ public: RSharedDescriptorGuard FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::DescriptorId_t &cid); /// Uses FindClusterId to search for the cluster with the entry index following the last entry index of the /// given cluster. - RSharedDescriptorGuard FindNextClusterId(ROOT::DescriptorId_t clusterId, ROOT::DescriptorId_t &nextId); + RAnyDescriptorGuard FindNextClusterId(ROOT::DescriptorId_t clusterId, ROOT::DescriptorId_t &nextId); /// Promise to only read from the given entry range. If set, prevents the cluster pool from reading-ahead beyond /// the given range. The range needs to be within `[0, GetNEntries())`. diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index cc8973e27ab0a..c1287dd9dc5cd 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -344,16 +344,20 @@ ROOT::NTupleSize_t ROOT::Internal::RPageSource::GetNElements(ROOT::DescriptorId_ return columnRange.GetFirstElementIndex() + columnRange.GetNElements(); } -ROOT::Internal::RPageSource::RSharedDescriptorGuard +ROOT::Internal::RPageSource::RAnyDescriptorGuard ROOT::Internal::RPageSource::FindNextClusterId(ROOT::DescriptorId_t clusterId, ROOT::DescriptorId_t &nextId) { - NTupleSize_t firstEntryInNextCluster = kInvalidNTupleIndex; - { - auto descriptorGuard = GetSharedDescriptorGuard(); - const auto &clusterDesc = descriptorGuard->GetClusterDescriptor(clusterId); - firstEntryInNextCluster = clusterDesc.GetFirstEntryIndex() + clusterDesc.GetNEntries(); + // At the cost of an additional cluster descriptor lock, we could use a more general approach that looks for the + // cluster containing the entry index after the last entry of the current cluster. + // At the moment, however, we know that cluster IDs are consecutive and clusters are not sharded. + auto sharedGuard = GetSharedDescriptorGuard(); + assert(clusterId < sharedGuard->GetNClusters()); + nextId = clusterId + 1; + if (nextId == sharedGuard->GetNClusters()) { + nextId = kInvalidDescriptorId; + return sharedGuard; } - return FindClusterId(firstEntryInNextCluster, nextId); + return EnsureClusterDetails(FindClusterGroupId(nextId), std::move(sharedGuard)); } ROOT::Internal::RPageSource::RSharedDescriptorGuard From 958ee367fbf137a7d8ff0cb39225b91fc46d826b Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 26 Sep 2026 22:42:18 +0200 Subject: [PATCH 16/24] [ntuple] properly lock entry-based RPageSource::FindClusterId() --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 2 +- tree/ntuple/src/RPageStorage.cxx | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index e1ca7fe6726cd..71de87b1e2a9d 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -965,7 +965,7 @@ public: RSharedDescriptorGuard FindClusterId(ROOT::DescriptorId_t physicalColumnId, ROOT::NTupleSize_t index, ROOT::DescriptorId_t &cid); /// An overload of FindClusterId that searches using a certain column element index. - RSharedDescriptorGuard FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::DescriptorId_t &cid); + RAnyDescriptorGuard FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::DescriptorId_t &cid); /// Uses FindClusterId to search for the cluster with the entry index following the last entry index of the /// given cluster. RAnyDescriptorGuard FindNextClusterId(ROOT::DescriptorId_t clusterId, ROOT::DescriptorId_t &nextId); diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index c1287dd9dc5cd..295a992c2c623 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -360,21 +360,20 @@ ROOT::Internal::RPageSource::FindNextClusterId(ROOT::DescriptorId_t clusterId, R return EnsureClusterDetails(FindClusterGroupId(nextId), std::move(sharedGuard)); } -ROOT::Internal::RPageSource::RSharedDescriptorGuard +ROOT::Internal::RPageSource::RAnyDescriptorGuard ROOT::Internal::RPageSource::FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::DescriptorId_t &cid) { cid = ROOT::kInvalidDescriptorId; - auto descGuard = GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + RAnyDescriptorGuard descGuard = GetSharedDescriptorGuard(); - if (desc.GetNClusterGroups() == 0) + if (descGuard->GetNClusterGroups() == 0) return descGuard; // Binary search in the cluster group list, followed by a binary search in the clusters of that cluster group - auto cgIter = desc.GetClusterGroupIterable().begin(); + const auto cgIter = descGuard->GetClusterGroupIterable().begin(); std::size_t cgLeft = 0; - std::size_t cgRight = desc.GetNClusterGroups() - 1; + std::size_t cgRight = descGuard->GetNClusterGroups() - 1; while (cgLeft <= cgRight) { const std::size_t cgMidpoint = (cgLeft + cgRight) / 2; const auto &cgDesc = *(cgIter + cgMidpoint); @@ -392,13 +391,15 @@ ROOT::Internal::RPageSource::FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::De // 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()); + const auto tgtClusterGroupId = cgDesc.GetId(); + descGuard = EnsureClusterDetails(tgtClusterGroupId, std::move(descGuard)); + + const auto &clusterIds = descGuard->GetClusterGroupDescriptor(tgtClusterGroupId).GetClusterIds(); 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 = desc.GetClusterDescriptor(clusterIds[clusterMidpoint]); + const auto &clusterDesc = descGuard->GetClusterDescriptor(clusterIds[clusterMidpoint]); if (clusterDesc.GetFirstEntryIndex() > entryIdx) { R__ASSERT(clusterMidpoint > 0); From 903a1f2c7534b2ff28fce9154d1cc8569b43ccac Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 26 Sep 2026 23:28:06 +0200 Subject: [PATCH 17/24] [ntuple] properly lock element-based RPageSource::FindClusterId() --- tree/ntuple/inc/ROOT/RPageStorage.hxx | 2 +- tree/ntuple/src/RPageStorage.cxx | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index 71de87b1e2a9d..a0b061f94e924 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -962,7 +962,7 @@ public: ROOT::NTupleSize_t GetNElements(ROOT::DescriptorId_t physicalColumnId); /// Returns a shared descriptor guard to ensure that the returned cluster id is useable, i.e. that the /// corresponding cluster was not meanwhile evicted from the set of active clusters. - RSharedDescriptorGuard + RAnyDescriptorGuard FindClusterId(ROOT::DescriptorId_t physicalColumnId, ROOT::NTupleSize_t index, ROOT::DescriptorId_t &cid); /// An overload of FindClusterId that searches using a certain column element index. RAnyDescriptorGuard FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::DescriptorId_t &cid); diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 295a992c2c623..7b63a236b9fe6 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -420,27 +420,28 @@ ROOT::Internal::RPageSource::FindClusterId(ROOT::NTupleSize_t entryIdx, ROOT::De return descGuard; } -ROOT::Internal::RPageSource::RSharedDescriptorGuard +ROOT::Internal::RPageSource::RAnyDescriptorGuard ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTupleSize_t index, DescriptorId_t &cid) { cid = ROOT::kInvalidDescriptorId; - auto descGuard = GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + RAnyDescriptorGuard descGuard = GetSharedDescriptorGuard(); - if (desc.GetNClusterGroups() == 0) + if (descGuard->GetNClusterGroups() == 0) return descGuard; // Binary search in the cluster group list, followed by a binary search in the clusters of that cluster group - auto cgIter = desc.GetClusterGroupIterable().begin(); std::size_t cgLeft = 0; - std::size_t cgRight = desc.GetNClusterGroups() - 1; + std::size_t cgRight = descGuard->GetNClusterGroups() - 1; while (cgLeft <= cgRight) { 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()); - const auto &clusterDesc = desc.GetClusterDescriptor(clusterIds.front()); + const auto &clusterDesc = descGuard->GetClusterDescriptor(clusterIds.front()); // this may happen if the RNTuple has an empty schema if (!clusterDesc.ContainsColumn(physicalColumnId)) return descGuard; @@ -453,7 +454,7 @@ ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTup continue; } - const auto &lastColumnRange = desc.GetClusterDescriptor(clusterIds.back()).GetColumnRange(physicalColumnId); + const auto &lastColumnRange = descGuard->GetClusterDescriptor(clusterIds.back()).GetColumnRange(physicalColumnId); if ((lastColumnRange.GetFirstElementIndex() + lastColumnRange.GetNElements()) <= index) { // Look into the upper half of cluster groups cgLeft = cgMidpoint + 1; @@ -467,7 +468,7 @@ ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTup while (clusterLeft <= clusterRight) { const std::size_t clusterMidpoint = (clusterLeft + clusterRight) / 2; const auto clusterId = clusterIds[clusterMidpoint]; - const auto &columnRange = desc.GetClusterDescriptor(clusterId).GetColumnRange(physicalColumnId); + const auto &columnRange = descGuard->GetClusterDescriptor(clusterId).GetColumnRange(physicalColumnId); if (columnRange.Contains(index)) { cid = clusterId; From 44409a058219817c915c16464ee64c734cf76ad1 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 3 Oct 2026 16:27:51 +0200 Subject: [PATCH 18/24] [ntuple] use FindClusterId() to get first cluster in merger --- tree/ntuple/src/RNTupleMerger.cxx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tree/ntuple/src/RNTupleMerger.cxx b/tree/ntuple/src/RNTupleMerger.cxx index b44247301b4af..e22ee7e7bdc85 100644 --- a/tree/ntuple/src/RNTupleMerger.cxx +++ b/tree/ntuple/src/RNTupleMerger.cxx @@ -194,9 +194,9 @@ try { // Get the compression of this RNTuple and use it as the output compression. // We currently assume all column ranges have the same compression, so we just peek at the first one. source->Attach(RNTupleSerializer::EDescriptorDeserializeMode::kRaw); - auto descGuard = source->GetSharedDescriptorGuard(); - auto clusterGroupIterable = descGuard->GetClusterGroupIterable(); - if (clusterGroupIterable.empty()) { + DescriptorId_t firstClusterId = kInvalidDescriptorId; + auto descGuard = source->FindClusterId(0, firstClusterId); + if (firstClusterId == kInvalidDescriptorId) { R__LOG_ERROR(NTupleMergeLog()) << "Asked to use the first source's compression as the output compression, but the " "first source (file '" @@ -205,9 +205,7 @@ try { "determined."; return -1; } - const auto firstClusterGroup = clusterGroupIterable.begin(); - R__ASSERT(firstClusterGroup->HasClusterDetails()); - const auto &firstCluster = descGuard->GetClusterDescriptor(firstClusterGroup->GetClusterIds()[0]); + const auto &firstCluster = descGuard->GetClusterDescriptor(firstClusterId); auto colRangeIter = firstCluster.GetColumnRangeIterable(); auto firstColRange = colRangeIter.begin(); if (firstColRange == colRangeIter.end()) { From 91ae775bfbc473b907cb8acc288eb9700c102bf4 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sun, 27 Sep 2026 23:49:06 +0200 Subject: [PATCH 19/24] [ntuple] add RPageSource::LoadAllPageLists() To be called after attach in cases where it is clear upfront that the full information of the cluster details is needed. --- .../io/hadd/merge_changeComp_check_output.C | 1 + tree/dataframe/src/RNTupleDS.cxx | 3 ++ tree/ntuple/inc/ROOT/RPageStorage.hxx | 2 ++ tree/ntuple/src/RNTupleMerger.cxx | 2 ++ tree/ntuple/src/RNTupleReader.cxx | 5 ++- tree/ntuple/src/RPageStorage.cxx | 34 ++++++++++++------- tree/ntuple/test/ntuple_storage_daos.cxx | 1 + tree/ntupleutil/src/RNTupleExporter.cxx | 1 + tree/ntupleutil/src/RNTupleInspector.cxx | 1 + 9 files changed, 36 insertions(+), 14 deletions(-) diff --git a/roottest/root/io/hadd/merge_changeComp_check_output.C b/roottest/root/io/hadd/merge_changeComp_check_output.C index cad0053cf1792..8d03dfbe8897c 100644 --- a/roottest/root/io/hadd/merge_changeComp_check_output.C +++ b/roottest/root/io/hadd/merge_changeComp_check_output.C @@ -23,6 +23,7 @@ int merge_changeComp_check_output(int expectedCompressionRNT, int expectedCompre ROOT::Internal::RPageSourceFile source("ntpl", fnameOut, ROOT::RNTupleReadOptions()); source.Attach(); + source.LoadAllPageLists(); ROOT::Internal::RClusterPool pool{source}; diff --git a/tree/dataframe/src/RNTupleDS.cxx b/tree/dataframe/src/RNTupleDS.cxx index 59ec6f0191ec5..dba784e56de57 100644 --- a/tree/dataframe/src/RNTupleDS.cxx +++ b/tree/dataframe/src/RNTupleDS.cxx @@ -552,6 +552,7 @@ void ROOT::RDF::RNTupleDS::PrepareNextRanges() source = CreatePageSource(fNTupleName, fFileNames[fNextFileIndex]); } source->Attach(); + source->LoadAllPageLists(); fNextFileIndex++; auto nEntries = source->GetNEntries(); @@ -891,6 +892,8 @@ ROOT::Internal::RDF::GetClustersAndEntries(std::string_view ntupleName, std::str { auto source = ROOT::Internal::RPageSource::Create(ntupleName, location); source->Attach(); + // TODO(jblomer): improve MT scheduling such that not all page lists are required right away + source->LoadAllPageLists(); auto descGuard = source->GetSharedDescriptorGuard(); return std::make_pair(ROOT::Internal::GetClusterBoundaries(descGuard.GetRef()), descGuard->GetNEntries()); } diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index a0b061f94e924..d9afb51df61c3 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -957,6 +957,8 @@ public: /// Open the physical storage container and deserialize header and footer void Attach(ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode mode = ROOT::Internal::RNTupleSerializer::EDescriptorDeserializeMode::kForReading); + /// Load all remaining cluster details + void LoadAllPageLists(); ROOT::NTupleSize_t GetNEntries(); ROOT::NTupleSize_t GetNElements(ROOT::DescriptorId_t physicalColumnId); diff --git a/tree/ntuple/src/RNTupleMerger.cxx b/tree/ntuple/src/RNTupleMerger.cxx index e22ee7e7bdc85..c055afa131054 100644 --- a/tree/ntuple/src/RNTupleMerger.cxx +++ b/tree/ntuple/src/RNTupleMerger.cxx @@ -232,6 +232,7 @@ try { if (outNTuple) { auto outSource = RPageSourceFile::CreateFromAnchor(*outNTuple); outSource->Attach(RNTupleSerializer::EDescriptorDeserializeMode::kForWriting); + outSource->LoadAllPageLists(); auto desc = outSource->GetSharedDescriptorGuard(); model = destination->InitFromDescriptor(desc.GetRef(), true /* copyClusters */); } @@ -995,6 +996,7 @@ class RNTupleMergeStrategyDefault final : public RNTupleMergeStrategy { void InitSource(RPageSource &source) final { source.Attach(RNTupleSerializer::EDescriptorDeserializeMode::kForWriting); + source.LoadAllPageLists(); fSrcDescriptor = &source.GetSharedDescriptorGuard().GetRef(); if (!fDstModel) { diff --git a/tree/ntuple/src/RNTupleReader.cxx b/tree/ntuple/src/RNTupleReader.cxx index 960f1e83a0478..7960749a21055 100644 --- a/tree/ntuple/src/RNTupleReader.cxx +++ b/tree/ntuple/src/RNTupleReader.cxx @@ -282,7 +282,10 @@ void ROOT::RNTupleReader::PrintInfo(const ENTupleInfo what, std::ostream &output output << std::flush; break; } - case ENTupleInfo::kStorageDetails: fSource->GetSharedDescriptorGuard()->PrintInfo(output); break; + case ENTupleInfo::kStorageDetails: + fSource->LoadAllPageLists(); + fSource->GetSharedDescriptorGuard()->PrintInfo(output); + break; case ENTupleInfo::kMetrics: fMetrics.Print(output); break; default: // Unhandled case, internal error diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 7b63a236b9fe6..a06a96c2fa8fe 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -242,25 +242,33 @@ void ROOT::Internal::RPageSource::Attach(RNTupleSerializer::EDescriptorDeseriali LoadStructure(); - auto descGuard = GetExclDescriptorGuard(); - descGuard.MoveIn(AttachImpl()); - fStructureBuffer.Reset(); - - NTupleSize_t sumClusterCount = 0; - fCumulativeClusterCounts.reserve(descGuard->GetNClusterGroups()); - for (const auto &cgDesc : descGuard->GetClusterGroupIterable()) { - sumClusterCount += cgDesc.GetNClusters(); - fCumulativeClusterCounts.emplace_back(sumClusterCount); + { + auto descGuard = GetExclDescriptorGuard(); + descGuard.MoveIn(AttachImpl()); + fStructureBuffer.Reset(); + + NTupleSize_t sumClusterCount = 0; + fCumulativeClusterCounts.reserve(descGuard->GetNClusterGroups()); + for (const auto &cgDesc : descGuard->GetClusterGroupIterable()) { + sumClusterCount += cgDesc.GetNClusters(); + fCumulativeClusterCounts.emplace_back(sumClusterCount); + } } - // For a descriptor coming from disk, we know that cluster group IDs are issued consecutively - for (DescriptorId_t cgId = 0; cgId < descGuard->GetNClusterGroups(); ++cgId) { - LoadPageList(cgId, descGuard); - } + LoadAllPageLists(); fIsAttached = true; } +void ROOT::Internal::RPageSource::LoadAllPageLists() +{ + auto descGuard = GetExclDescriptorGuard(); + + for (const auto &cgDesc : descGuard->GetClusterGroupIterable()) { + LoadPageList(cgDesc.GetId(), descGuard); + } +} + void ROOT::Internal::RPageSource::LoadPageList(DescriptorId_t clusterGroupId, const RExclDescriptorGuard &exclGuard) { const auto &cgDesc = exclGuard->GetClusterGroupDescriptor(clusterGroupId); diff --git a/tree/ntuple/test/ntuple_storage_daos.cxx b/tree/ntuple/test/ntuple_storage_daos.cxx index 0c91bacc8887f..dcfd2909b5fb3 100644 --- a/tree/ntuple/test/ntuple_storage_daos.cxx +++ b/tree/ntuple/test/ntuple_storage_daos.cxx @@ -324,6 +324,7 @@ TEST_F(RPageStorageDaos, Checksum) ROOT::DescriptorId_t clusterId; auto pageSource = RPageSource::Create("ntpl", daosUri); pageSource->Attach(); + pageSource->LoadAllPageLists(); { auto descGuard = pageSource->GetSharedDescriptorGuard(); pxColId = descGuard->FindPhysicalColumnId(descGuard->FindFieldId("px"), 0, 0); diff --git a/tree/ntupleutil/src/RNTupleExporter.cxx b/tree/ntupleutil/src/RNTupleExporter.cxx index 487863349cd9c..4816af6ee2262 100644 --- a/tree/ntupleutil/src/RNTupleExporter.cxx +++ b/tree/ntupleutil/src/RNTupleExporter.cxx @@ -118,6 +118,7 @@ RNTupleExporter::ExportPages(ROOT::Internal::RPageSource &source, const RPagesOp // make sure the source is attached source.Attach(); + source.LoadAllPageLists(); auto desc = source.GetSharedDescriptorGuard(); ROOT::Internal::RClusterPool clusterPool{source}; diff --git a/tree/ntupleutil/src/RNTupleInspector.cxx b/tree/ntupleutil/src/RNTupleInspector.cxx index ad50768d0ffe8..1b1ac6a66e71e 100644 --- a/tree/ntupleutil/src/RNTupleInspector.cxx +++ b/tree/ntupleutil/src/RNTupleInspector.cxx @@ -35,6 +35,7 @@ ROOT::Experimental::RNTupleInspector::RNTupleInspector(std::unique_ptrAttach(); + fPageSource->LoadAllPageLists(); auto descriptorGuard = fPageSource->GetSharedDescriptorGuard(); fDescriptor = descriptorGuard->Clone(); From a7ec7c7f330f138070945bc7ac2288a8e79755ca Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 7 Oct 2026 21:18:28 +0200 Subject: [PATCH 20/24] [ntuple] lock access to storage backend With on-demand loading of page lists, the connection to storage (file handle, DAOS container, etc.) can be used concurrently, by the page list loading and by the prefetcher. Hence requires locking. --- tree/ntuple/inc/ROOT/RPageStorageDaos.hxx | 3 +++ tree/ntuple/inc/ROOT/RPageStorageFile.hxx | 3 +++ tree/ntuple/src/RPageStorageDaos.cxx | 4 ++++ tree/ntuple/src/RPageStorageFile.cxx | 9 ++++++--- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorageDaos.hxx b/tree/ntuple/inc/ROOT/RPageStorageDaos.hxx index b5ff59bddd0e6..66b33b121d43d 100644 --- a/tree/ntuple/inc/ROOT/RPageStorageDaos.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorageDaos.hxx @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -151,6 +152,8 @@ private: /// A container that stores object data (header/footer, pages, etc.) std::unique_ptr fDaosContainer; + /// Protects the DAOS container + std::mutex fContainerLock; /// A URI to a DAOS pool of the form 'daos://pool-label/container-label' std::string fURI; diff --git a/tree/ntuple/inc/ROOT/RPageStorageFile.hxx b/tree/ntuple/inc/ROOT/RPageStorageFile.hxx index d94feff68f1e7..c0db553496be8 100644 --- a/tree/ntuple/inc/ROOT/RPageStorageFile.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorageFile.hxx @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -139,6 +140,8 @@ private: RNTupleDescriptorBuilder fDescriptorBuilder; /// Tracks the last read offset for seek distance calculation std::uint64_t fLastOffset = 0; + /// Protects the raw file, the reader, and fLastOffset + std::mutex fReaderLock; /// File-specific I/O performance counters struct RFileCounters { diff --git a/tree/ntuple/src/RPageStorageDaos.cxx b/tree/ntuple/src/RPageStorageDaos.cxx index 30329c778e1b8..c619bfa1ede52 100644 --- a/tree/ntuple/src/RPageStorageDaos.cxx +++ b/tree/ntuple/src/RPageStorageDaos.cxx @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -481,6 +482,7 @@ ROOT::RNTupleDescriptor ROOT::Experimental::Internal::RPageSourceDaos::AttachImp void ROOT::Experimental::Internal::RPageSourceDaos::LoadPageListImpl(const RNTupleLocator &locator, unsigned char *buffer) { + std::lock_guard lockGuard(fContainerLock); daos_obj_id_t oidPageList{kOidLowPageList, static_cast(fNTupleIndex)}; fDaosContainer->ReadSingleAkey(buffer, locator.GetNBytesOnStorage(), oidPageList, kDistributionKeyDefault, locator.GetPosition().GetLocation()); @@ -494,6 +496,7 @@ std::string ROOT::Experimental::Internal::RPageSourceDaos::GetObjectClass() cons void ROOT::Experimental::Internal::RPageSourceDaos::LoadSealedPageImpl(const RNTupleLocator &locator, RSealedPage &sealedPage) { + std::lock_guard lockGuard(fContainerLock); RDaosKey daosKey = GetPageDaosKey(fNTupleIndex, locator.GetPosition().GetLocation()); fDaosContainer->ReadSingleAkey(const_cast(sealedPage.GetBuffer()), sealedPage.GetBufferSize(), daosKey.fOid, daosKey.fDkey, daosKey.fAkey); @@ -587,6 +590,7 @@ ROOT::Experimental::Internal::RPageSourceDaos::LoadClusters(std::spanfTimeWallRead, fCounters->fTimeCpuRead); + std::lock_guard lockGuard(fContainerLock); if (int err = fDaosContainer->ReadV(readRequests)) throw ROOT::RException(R__FAIL("ReadV: error" + std::string(d_errstr(err)))); } diff --git a/tree/ntuple/src/RPageStorageFile.cxx b/tree/ntuple/src/RPageStorageFile.cxx index 5a6749b0d953a..5dfe5baa84ed0 100644 --- a/tree/ntuple/src/RPageStorageFile.cxx +++ b/tree/ntuple/src/RPageStorageFile.cxx @@ -34,12 +34,11 @@ #include #include #include +#include #include #include -#include - -#include #include +#include using ROOT::Experimental::Detail::RNTupleAtomicCounter; using ROOT::Experimental::Detail::RNTupleAtomicTimer; @@ -493,6 +492,7 @@ ROOT::RNTupleDescriptor ROOT::Internal::RPageSourceFile::AttachImpl() void ROOT::Internal::RPageSourceFile::LoadPageListImpl(const RNTupleLocator &locator, unsigned char *buffer) { + std::lock_guard lockGuard(fReaderLock); fReader.ReadBuffer(buffer, locator.GetNBytesOnStorage(), locator.GetPosition()); } @@ -500,6 +500,7 @@ void ROOT::Internal::RPageSourceFile::LoadSealedPageImpl(const RNTupleLocator &l { RNTupleAtomicTimer timer(fCounters->fTimeWallRead, fCounters->fTimeCpuRead); const auto offset = locator.GetPosition(); + std::lock_guard lockGuard(fReaderLock); // Track seek distance (excluding file structure reads) if (fLastOffset != 0) { R__ASSERT(fFileCounters); @@ -683,6 +684,8 @@ ROOT::Internal::RPageSourceFile::LoadClusters(std::span clusterK } } + std::lock_guard lockGuard(fReaderLock); + // Track seek distance for each read request (excluding file structure reads) R__ASSERT(fFileCounters); for (std::size_t i = 0; i < nBatch; ++i) { From 1219fc4395322024b30de9c735968c7e734e0767 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 28 Sep 2026 11:35:49 +0200 Subject: [PATCH 21/24] [ntuple] add metadata mode read option --- .../test/dataframe_snapshot_ntuple.cxx | 12 +++- tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx | 20 ++++++ tree/ntuple/src/RPageStorage.cxx | 3 +- tree/ntuple/test/ntuple_basics.cxx | 4 +- tree/ntuple/test/ntuple_checksum.cxx | 8 ++- tree/ntuple/test/ntuple_descriptor.cxx | 8 ++- tree/ntuple/test/ntuple_extended.cxx | 4 +- tree/ntuple/test/ntuple_limits.cxx | 9 ++- tree/ntuple/test/ntuple_merger.cxx | 4 +- tree/ntuple/test/ntuple_multi_column.cxx | 4 +- tree/ntuple/test/ntuple_parallel_writer.cxx | 8 ++- tree/ntuple/test/ntuple_storage.cxx | 67 +++++++++++++++++-- tree/ntuple/test/ntuple_storage_daos.cxx | 4 +- tree/ntuple/test/ntuple_storage_s3.cxx | 4 +- tree/ntuple/test/ntuple_test.cxx | 4 +- tree/ntuple/test/rfield_string.cxx | 4 +- 16 files changed, 140 insertions(+), 27 deletions(-) diff --git a/tree/dataframe/test/dataframe_snapshot_ntuple.cxx b/tree/dataframe/test/dataframe_snapshot_ntuple.cxx index d4e92e393a823..b6428d4713895 100644 --- a/tree/dataframe/test/dataframe_snapshot_ntuple.cxx +++ b/tree/dataframe/test/dataframe_snapshot_ntuple.cxx @@ -125,7 +125,9 @@ TEST(RDFSnapshotRNTuple, WriteOpts) EXPECT_EQ(columns, sdf->GetColumnNames()); - auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath()); + ROOT::RNTupleReadOptions ntplOpts; + ntplOpts.SetMetadataMode(ROOT::RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath(), ntplOpts); EXPECT_EQ(2, reader->GetDescriptor().GetNClusters()); EXPECT_EQ(2, reader->GetDescriptor().GetClusterDescriptor(0).GetPageRange(0).GetPageInfos().size()); EXPECT_EQ(2, reader->GetDescriptor().GetClusterDescriptor(1).GetPageRange(0).GetPageInfos().size()); @@ -184,7 +186,9 @@ TEST(RDFSnapshotRNTuple, DefaultCompressionSettings) EXPECT_EQ(columns, sdf->GetColumnNames()); - auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath()); + ROOT::RNTupleReadOptions ntplOpts; + ntplOpts.SetMetadataMode(ROOT::RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath(), ntplOpts); auto compSettings = *reader->GetDescriptor().GetClusterDescriptor(0).GetColumnRange(0).GetCompressionSettings(); // The RNTuple default should be 505 EXPECT_EQ(505, compSettings); @@ -206,7 +210,9 @@ TEST(RDFSnapshotRNTuple, Compression) EXPECT_EQ(columns, sdf->GetColumnNames()); - auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath()); + ROOT::RNTupleReadOptions ntplOpts; + ntplOpts.SetMetadataMode(ROOT::RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath(), ntplOpts); auto compSettings = *reader->GetDescriptor().GetClusterDescriptor(0).GetColumnRange(0).GetCompressionSettings(); EXPECT_EQ(404, compSettings); } diff --git a/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx b/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx index 442756ce48bab..807afa7046642 100644 --- a/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx +++ b/tree/ntuple/inc/ROOT/RNTupleReadOptions.hxx @@ -66,6 +66,16 @@ This is useful, e.g., in the context of RDataFrame where the threads are fully m + +`EMetadataMode` +EMetadataMode +EMetadataMode::kDefault + +Set whether cluster metadata (page locations) are loaded entirely on open or on demand during entry iteration. +By default, the RNTuple reader loads metadata on demand. + + + `EnableMetrics` `bool` @@ -93,12 +103,19 @@ public: kDefault, }; + enum class EMetadataMode { + kEager, + kOnDemand, + kDefault = kOnDemand, + }; + private: EClusterCache fClusterCache = EClusterCache::kDefault; /// 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; EImplicitMT fUseImplicitMT = EImplicitMT::kDefault; + EMetadataMode fMetadataMode = EMetadataMode::kDefault; bool fEnableMetrics = false; /// Largest header or footer envelope a page source will accept from an anchor it does not fully trust, /// such as the S3 anchor, which is a separately stored object. Guards the allocation made from those sizes. @@ -111,6 +128,9 @@ public: EImplicitMT GetUseImplicitMT() const { return fUseImplicitMT; } void SetUseImplicitMT(EImplicitMT val) { fUseImplicitMT = val; } + EMetadataMode GetMetadataMode() const { return fMetadataMode; } + void SetMetadataMode(EMetadataMode val) { fMetadataMode = val; } + bool GetEnableMetrics() const { return fEnableMetrics; } void SetEnableMetrics(bool val) { fEnableMetrics = val; } }; // class RNTupleReadOptions diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index a06a96c2fa8fe..768c241ca6b43 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -255,7 +255,8 @@ void ROOT::Internal::RPageSource::Attach(RNTupleSerializer::EDescriptorDeseriali } } - LoadAllPageLists(); + if (fOptions.GetMetadataMode() == RNTupleReadOptions::EMetadataMode::kEager) + LoadAllPageLists(); fIsAttached = true; } diff --git a/tree/ntuple/test/ntuple_basics.cxx b/tree/ntuple/test/ntuple_basics.cxx index 1737711744d38..5b50092f87f82 100644 --- a/tree/ntuple/test/ntuple_basics.cxx +++ b/tree/ntuple/test/ntuple_basics.cxx @@ -446,7 +446,9 @@ TEST(RNTuple, PageSize) } } - auto ntuple = RNTupleReader::Open("ntuple", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto ntuple = RNTupleReader::Open("ntuple", fileGuard.GetPath(), opts); const auto &col0_pages = ntuple->GetDescriptor().GetClusterDescriptor(0).GetPageRange(0); // 1000 column elements / 50 elements per page EXPECT_EQ(20, col0_pages.GetPageInfos().size()); diff --git a/tree/ntuple/test/ntuple_checksum.cxx b/tree/ntuple/test/ntuple_checksum.cxx index a650fc6e9d302..9bf065cdf2aa7 100644 --- a/tree/ntuple/test/ntuple_checksum.cxx +++ b/tree/ntuple/test/ntuple_checksum.cxx @@ -61,7 +61,9 @@ TEST(RNTupleChecksum, VerifyOnLoad) ROOT::DescriptorId_t pyColId; ROOT::DescriptorId_t pzColId; ROOT::DescriptorId_t clusterId; - auto pageSource = RPageSource::Create("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto pageSource = RPageSource::Create("ntpl", fileGuard.GetPath(), opts); pageSource->Attach(); { auto descGuard = pageSource->GetSharedDescriptorGuard(); @@ -97,7 +99,9 @@ TEST(RNTupleChecksum, OmitPageChecksum) writer->Fill(); writer.reset(); - auto pageSource = RPageSource::Create("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto pageSource = RPageSource::Create("ntpl", fileGuard.GetPath(), opts); pageSource->Attach(); auto descGuard = pageSource->GetSharedDescriptorGuard(); const auto pxColId = descGuard->FindPhysicalColumnId(descGuard->FindFieldId("px"), 0, 0); diff --git a/tree/ntuple/test/ntuple_descriptor.cxx b/tree/ntuple/test/ntuple_descriptor.cxx index 8367608bae62a..bc982f67caf44 100644 --- a/tree/ntuple/test/ntuple_descriptor.cxx +++ b/tree/ntuple/test/ntuple_descriptor.cxx @@ -633,7 +633,9 @@ TEST(RClusterGroupDescriptorIterable, Ordering) RNTupleWriter::Append(RNTupleModel::Create(), "empty", *f); } - auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath(), opts); const auto &desc = reader->GetDescriptor(); EXPECT_EQ(3u, desc.GetNClusterGroups()); @@ -674,7 +676,9 @@ TEST(RClusterDescriptor, GetNBytesOnStorage) writer->Fill(); } - auto ntuple = RNTupleReader::Open("ntuple", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto ntuple = RNTupleReader::Open("ntuple", fileGuard.GetPath(), opts); const auto &desc = ntuple->GetDescriptor(); EXPECT_EQ(8 + 8 + 8 + 3, desc.GetActiveClusterIterable().begin()->GetNBytesOnStorage()); } diff --git a/tree/ntuple/test/ntuple_extended.cxx b/tree/ntuple/test/ntuple_extended.cxx index d7a0fee9ac9ab..4874f14ccf6a2 100644 --- a/tree/ntuple/test/ntuple_extended.cxx +++ b/tree/ntuple/test/ntuple_extended.cxx @@ -181,7 +181,9 @@ TEST(RNTuple, LargePages) writer.reset(); } - auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); const auto &desc = reader->GetDescriptor(); const auto rndColId = desc.FindPhysicalColumnId(desc.FindFieldId("rnd"), 0, 0); const auto &clusterDesc = *desc.GetActiveClusterIterable().begin(); diff --git a/tree/ntuple/test/ntuple_limits.cxx b/tree/ntuple/test/ntuple_limits.cxx index 9b37e29611b17..ed769ae4b93f4 100644 --- a/tree/ntuple/test/ntuple_limits.cxx +++ b/tree/ntuple/test/ntuple_limits.cxx @@ -77,7 +77,6 @@ TEST(RNTuple, Limits_ManyClusters) EXPECT_EQ(reader->GetNEntries(), NumClusters); EXPECT_EQ(descriptor.GetNClusters(), NumClusters); - EXPECT_EQ(descriptor.GetNActiveClusters(), NumClusters); auto id = model.GetDefaultEntry().GetPtr("id"); for (int i = 0; i < NumClusters; i++) { @@ -147,7 +146,9 @@ TEST(RNTuple, Limits_ManyPages) } } - auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath(), opts); const auto &descriptor = reader->GetDescriptor(); const auto &model = reader->GetModel(); auto fieldId = descriptor.FindFieldId("id"); @@ -189,7 +190,9 @@ TEST(RNTuple, Limits_ManyPagesOneEntry) writer->Fill(); } - auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("myNTuple", fileGuard.GetPath(), opts); const auto &descriptor = reader->GetDescriptor(); const auto &model = reader->GetModel(); auto fieldId = descriptor.FindFieldId("ids"); diff --git a/tree/ntuple/test/ntuple_merger.cxx b/tree/ntuple/test/ntuple_merger.cxx index 73267ea9028da..27287d804d165 100644 --- a/tree/ntuple/test/ntuple_merger.cxx +++ b/tree/ntuple/test/ntuple_merger.cxx @@ -826,7 +826,9 @@ static bool VerifyPageCompression(const std::string_view fileName, std::uint32_t // Check that the advertised compression is correct bool ok = true; { - auto reader = RNTupleReader::Open("ntuple", fileName); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntuple", fileName, opts); auto compSettings = *reader->GetDescriptor().GetClusterDescriptor(0).GetColumnRange(0).GetCompressionSettings(); if (compSettings != expectedComp) { std::cerr << "Advertised compression is wrong: " << compSettings << " instead of " << expectedComp << "\n"; diff --git a/tree/ntuple/test/ntuple_multi_column.cxx b/tree/ntuple/test/ntuple_multi_column.cxx index 963c5fabd9277..80f0cb474d49a 100644 --- a/tree/ntuple/test/ntuple_multi_column.cxx +++ b/tree/ntuple/test/ntuple_multi_column.cxx @@ -27,7 +27,9 @@ TEST(RNTuple, MultiColumnRepresentationSimple) writer->Fill(); } - auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); EXPECT_EQ(3u, reader->GetView("px").GetFieldRange().size()); const auto &desc = reader->GetDescriptor(); diff --git a/tree/ntuple/test/ntuple_parallel_writer.cxx b/tree/ntuple/test/ntuple_parallel_writer.cxx index ebb04cd977ebd..23f94108f8ff2 100644 --- a/tree/ntuple/test/ntuple_parallel_writer.cxx +++ b/tree/ntuple/test/ntuple_parallel_writer.cxx @@ -262,7 +262,9 @@ TEST(RNTupleParallelWriter, StagedMultiColumn) c->CommitStagedClusters(); } - auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); EXPECT_EQ(3u, reader->GetView("px").GetFieldRange().size()); const auto &desc = reader->GetDescriptor(); @@ -417,7 +419,9 @@ TEST(RNTupleFillContext, FlushColumns) } // If FlushColumns() worked, there will be two pages with one element each. - auto reader = RNTupleReader::Open("f", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("f", fileGuard.GetPath(), opts); const auto &descriptor = reader->GetDescriptor(); auto fieldId = descriptor.FindFieldId("pt"); diff --git a/tree/ntuple/test/ntuple_storage.cxx b/tree/ntuple/test/ntuple_storage.cxx index 3021278c81ab0..da8c0156daf21 100644 --- a/tree/ntuple/test/ntuple_storage.cxx +++ b/tree/ntuple/test/ntuple_storage.cxx @@ -214,7 +214,9 @@ TEST(RNTuple, PageFilling) writer->Fill(); } - auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); const auto &desc = reader->GetDescriptor(); EXPECT_EQ(1u, desc.GetNClusters()); const auto colIdX = desc.FindLogicalColumnId(desc.FindFieldId("x"), 0, 0); @@ -262,7 +264,9 @@ TEST(RNTuple, PageFillingString) ntuple->Fill(); // main write page is half full here; RColumn::AppendV should flush the shadow page } - auto ntuple = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto ntuple = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); auto viewX = ntuple->GetView("x"); ASSERT_EQ(5u, ntuple->GetNEntries()); EXPECT_EQ("01234567890123456", viewX(0)); @@ -317,7 +321,9 @@ TEST(RNTuple, FlushColumns) } // If FlushColumns() worked, there will be two pages with one element each. - auto reader = RNTupleReader::Open("f", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("f", fileGuard.GetPath(), opts); const auto &descriptor = reader->GetDescriptor(); auto fieldId = descriptor.FindFieldId("pt"); @@ -924,8 +930,10 @@ TEST(RPageStorageFile, MultiKeyBlob_Pages) auto iU = modelUcmp->MakeField("i"); auto fC = modelComp->MakeField("f"); auto iC = modelComp->MakeField("i"); - auto ntupleUcmp = RNTupleReader::Open(std::move(modelUcmp), "myNTuple", fileGuardUcmp.GetPath()); - auto ntupleComp = RNTupleReader::Open(std::move(modelComp), "myNTuple", fileGuardComp.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto ntupleUcmp = RNTupleReader::Open(std::move(modelUcmp), "myNTuple", fileGuardUcmp.GetPath(), opts); + auto ntupleComp = RNTupleReader::Open(std::move(modelComp), "myNTuple", fileGuardComp.GetPath(), opts); // Verify that the pages are larger than maxKeySize EXPECT_GT(ntupleComp->GetDescriptor() @@ -1099,7 +1107,9 @@ TEST(RPageSink, SamePageMerging) writer->Fill(); writer.reset(); - auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); EXPECT_EQ(1u, reader->GetNEntries()); const auto &desc = reader->GetDescriptor(); @@ -1305,3 +1315,48 @@ TEST(RPageSink, AddColumnRepresentationWithData) EXPECT_EQ(col1.GetType(), ROOT::ENTupleColumnType::kInt32); EXPECT_EQ(col2.GetType(), ROOT::ENTupleColumnType::kSplitInt32); } + +TEST(RPageSource, MetadataMode) +{ + FileRaii fileGuard("test_ntuple_page_source_metadata_mode.root"); + + auto model = RNTupleModel::Create(); + auto ptrCtr = model->MakeField("ctr"); + { + auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); + for (int i = 0; i < 1000; ++i) { + *ptrCtr = i; + writer->Fill(); + writer->CommitCluster(true /* commit cluster group */); + } + } + + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); + EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusterGroups()); + EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusters()); + EXPECT_EQ(1000u, reader->GetDescriptor().GetNActiveClusters()); + + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kDefault); + reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); + EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusterGroups()); + EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusters()); + EXPECT_EQ(0u, reader->GetDescriptor().GetNActiveClusters()); + + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kOnDemand); + reader = RNTupleReader::Open("ntpl", fileGuard.GetPath(), opts); + EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusterGroups()); + EXPECT_EQ(1000u, reader->GetDescriptor().GetNClusters()); + EXPECT_EQ(0u, reader->GetDescriptor().GetNActiveClusters()); + + ptrCtr = reader->GetModel().GetDefaultEntry().GetPtr("ctr"); + reader->LoadEntry(500); + EXPECT_EQ(500, *ptrCtr); + reader->LoadEntry(999); + EXPECT_EQ(999, *ptrCtr); + reader->LoadEntry(0); + EXPECT_EQ(0, *ptrCtr); + EXPECT_LT(0, reader->GetDescriptor().GetNActiveClusters()); + EXPECT_GT(1000, reader->GetDescriptor().GetNActiveClusters()); +} diff --git a/tree/ntuple/test/ntuple_storage_daos.cxx b/tree/ntuple/test/ntuple_storage_daos.cxx index dcfd2909b5fb3..706cbec4fd11a 100644 --- a/tree/ntuple/test/ntuple_storage_daos.cxx +++ b/tree/ntuple/test/ntuple_storage_daos.cxx @@ -275,7 +275,9 @@ TEST_F(RPageStorageDaos, DisabledSamePageMerging) writer->Fill(); writer.reset(); - auto reader = RNTupleReader::Open("ntpl", daosUri); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto reader = RNTupleReader::Open("ntpl", daosUri, opts); EXPECT_EQ(1u, reader->GetNEntries()); const auto &desc = reader->GetDescriptor(); diff --git a/tree/ntuple/test/ntuple_storage_s3.cxx b/tree/ntuple/test/ntuple_storage_s3.cxx index b2faf0a43ccac..c3d6251f0b68b 100644 --- a/tree/ntuple/test/ntuple_storage_s3.cxx +++ b/tree/ntuple/test/ntuple_storage_s3.cxx @@ -1052,7 +1052,9 @@ TEST_F(RPageSourceS3Wire, RoundTripManyPagesPerCluster) } } - auto reader = ROOT::RNTupleReader::Open("manypages", uri); + ROOT::RNTupleReadOptions opts; + opts.SetMetadataMode(ROOT::RNTupleReadOptions::EMetadataMode::kEager); + auto reader = ROOT::RNTupleReader::Open("manypages", uri, opts); EXPECT_EQ(1000u, reader->GetNEntries()); // Guard the premise of the test: one cluster, many pages per column. diff --git a/tree/ntuple/test/ntuple_test.cxx b/tree/ntuple/test/ntuple_test.cxx index 5e2e9bd6a4a2e..caef0abfd3163 100644 --- a/tree/ntuple/test/ntuple_test.cxx +++ b/tree/ntuple/test/ntuple_test.cxx @@ -32,7 +32,9 @@ void CreateCorruptedRNTuple(const std::string &uri) writer.reset(); // Load sealed pages to memory - auto pageSource = RPageSource::Create("ntpl", uri); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto pageSource = RPageSource::Create("ntpl", uri, opts); pageSource->Attach(); auto descGuard = pageSource->GetSharedDescriptorGuard(); const auto pxColId = descGuard->FindPhysicalColumnId(descGuard->FindFieldId("px"), 0, 0); diff --git a/tree/ntuple/test/rfield_string.cxx b/tree/ntuple/test/rfield_string.cxx index 7a2b7343519cb..c45d5f043255a 100644 --- a/tree/ntuple/test/rfield_string.cxx +++ b/tree/ntuple/test/rfield_string.cxx @@ -19,7 +19,9 @@ TEST(RNTuple, ReadString) } } - auto ntuple = RNTupleReader::Open(ntupleName, fileGuard.GetPath()); + RNTupleReadOptions opts; + opts.SetMetadataMode(RNTupleReadOptions::EMetadataMode::kEager); + auto ntuple = RNTupleReader::Open(ntupleName, fileGuard.GetPath(), opts); auto viewSt = ntuple->GetView("st"); if (ntuple->GetDescriptor().GetClusterDescriptor(0).GetPageRange(1).GetPageInfos().size() < 2) { FAIL(); // This means all entries are inside the same page and numEntries should be increased. From 86decd7c022d213cf5fb1c18b6948170b167979e Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Tue, 29 Sep 2026 23:08:25 +0200 Subject: [PATCH 22/24] [ntuple] remove dead code in FindClusterId() --- tree/ntuple/src/RPageStorage.cxx | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 768c241ca6b43..caf2ef81650cc 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -450,13 +450,9 @@ ROOT::Internal::RPageSource::FindClusterId(DescriptorId_t physicalColumnId, NTup const auto &clusterIds = (cgIter + cgMidpoint)->GetClusterIds(); R__ASSERT(!clusterIds.empty()); - const auto &clusterDesc = descGuard->GetClusterDescriptor(clusterIds.front()); - // this may happen if the RNTuple has an empty schema - if (!clusterDesc.ContainsColumn(physicalColumnId)) - return descGuard; - - const auto firstElementInGroup = clusterDesc.GetColumnRange(physicalColumnId).GetFirstElementIndex(); - if (firstElementInGroup > index) { + 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; From 8c58bd9afb89908f8a8b4944063ff84b6739361d Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sun, 4 Oct 2026 13:38:58 +0200 Subject: [PATCH 23/24] [ntuple] descriptor guard GetRef() --> GetConstRef() --- tree/dataframe/src/RNTupleDS.cxx | 4 ++-- tree/ntuple/inc/ROOT/RNTupleView.hxx | 6 +++--- tree/ntuple/inc/ROOT/RPageStorage.hxx | 6 +++--- tree/ntuple/src/RFieldBase.cxx | 4 ++-- tree/ntuple/src/RFieldMeta.cxx | 15 ++++++++------- tree/ntuple/src/RNTupleMerger.cxx | 4 ++-- tree/ntuple/src/RNTupleProcessor.cxx | 4 ++-- tree/ntuple/src/RNTupleView.cxx | 2 +- tree/ntupleutil/src/RNTupleExporter.cxx | 5 +++-- 9 files changed, 26 insertions(+), 24 deletions(-) diff --git a/tree/dataframe/src/RNTupleDS.cxx b/tree/dataframe/src/RNTupleDS.cxx index dba784e56de57..135dd04f18ac8 100644 --- a/tree/dataframe/src/RNTupleDS.cxx +++ b/tree/dataframe/src/RNTupleDS.cxx @@ -566,7 +566,7 @@ void ROOT::RDF::RNTupleDS::PrepareNextRanges() const auto rangesByCluster = [&source]() { // Take the shared lock of the descriptor just for the time necessary auto descGuard = source->GetSharedDescriptorGuard(); - return ROOT::Internal::GetClusterBoundaries(descGuard.GetRef()); + return ROOT::Internal::GetClusterBoundaries(descGuard.GetConstRef()); }(); const unsigned int nRangesByCluster = rangesByCluster.size(); @@ -895,5 +895,5 @@ ROOT::Internal::RDF::GetClustersAndEntries(std::string_view ntupleName, std::str // TODO(jblomer): improve MT scheduling such that not all page lists are required right away source->LoadAllPageLists(); auto descGuard = source->GetSharedDescriptorGuard(); - return std::make_pair(ROOT::Internal::GetClusterBoundaries(descGuard.GetRef()), descGuard->GetNEntries()); + return std::make_pair(ROOT::Internal::GetClusterBoundaries(descGuard.GetConstRef()), descGuard->GetNEntries()); } diff --git a/tree/ntuple/inc/ROOT/RNTupleView.hxx b/tree/ntuple/inc/ROOT/RNTupleView.hxx index 592357f9fc187..f0bc91aa2f923 100644 --- a/tree/ntuple/inc/ROOT/RNTupleView.hxx +++ b/tree/ntuple/inc/ROOT/RNTupleView.hxx @@ -93,7 +93,7 @@ protected: std::unique_ptr field; { auto descGuard = pageSource.GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + const auto &desc = descGuard.GetConstRef(); const auto &fieldDesc = desc.GetFieldDescriptor(fieldId); if constexpr (std::is_void_v) { if (typeName.empty()) @@ -275,7 +275,7 @@ protected: static ROOT::RField CreateField(ROOT::DescriptorId_t fieldId, ROOT::Internal::RPageSource &pageSource) { auto descGuard = pageSource.GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + const auto &desc = descGuard.GetConstRef(); const auto &fieldDesc = desc.GetFieldDescriptor(fieldId); if (!Internal::IsMatchingFieldType(fieldDesc.GetTypeName())) { throw RException(R__FAIL("type mismatch for field " + fieldDesc.GetFieldName() + ": " + @@ -340,7 +340,7 @@ private: std::string fieldName; { auto descGuard = source->GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + const auto &desc = descGuard.GetConstRef(); const auto &fieldDesc = desc.GetFieldDescriptor(fieldId); if (fieldDesc.GetStructure() != ROOT::ENTupleStructure::kCollection) { throw RException( diff --git a/tree/ntuple/inc/ROOT/RPageStorage.hxx b/tree/ntuple/inc/ROOT/RPageStorage.hxx index d9afb51df61c3..94c3dec969aff 100644 --- a/tree/ntuple/inc/ROOT/RPageStorage.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorage.hxx @@ -647,7 +647,7 @@ public: Release(); } const ROOT::RNTupleDescriptor *operator->() const { return fDescriptor; } - const ROOT::RNTupleDescriptor &GetRef() const { return *fDescriptor; } + const ROOT::RNTupleDescriptor &GetConstRef() const { return *fDescriptor; } bool IsValid() const { return fLock; } void Release() { @@ -715,9 +715,9 @@ public: RAnyDescriptorGuard &operator=(RAnyDescriptorGuard &&) = default; ~RAnyDescriptorGuard() = default; - const ROOT::RNTupleDescriptor &GetRef() const + const ROOT::RNTupleDescriptor &GetConstRef() const { - return (fAnyGuard.index() == 0) ? std::get<0>(fAnyGuard).GetRef() : std::get<1>(fAnyGuard).operator*(); + return (fAnyGuard.index() == 0) ? std::get<0>(fAnyGuard).GetConstRef() : std::get<1>(fAnyGuard).operator*(); } const ROOT::RNTupleDescriptor *operator->() const { diff --git a/tree/ntuple/src/RFieldBase.cxx b/tree/ntuple/src/RFieldBase.cxx index 05a5254c7d8d5..4762beeeceef8 100644 --- a/tree/ntuple/src/RFieldBase.cxx +++ b/tree/ntuple/src/RFieldBase.cxx @@ -1048,7 +1048,7 @@ void ROOT::RFieldBase::ConnectPageSource(ROOT::Internal::RPageSource &pageSource if (!fIsArtificial) { auto descGuard = pageSource.GetSharedDescriptorGuard(); - ReconcileOnDiskField(descGuard.GetRef()); + ReconcileOnDiskField(descGuard.GetConstRef()); } for (auto &f : fSubfields) { @@ -1061,7 +1061,7 @@ void ROOT::RFieldBase::ConnectPageSource(ROOT::Internal::RPageSource &pageSource // Do not generate columns nor set fColumnRepresentatives for artificial fields. if (!fIsArtificial) { const auto descriptorGuard = pageSource.GetSharedDescriptorGuard(); - const ROOT::RNTupleDescriptor &desc = descriptorGuard.GetRef(); + const ROOT::RNTupleDescriptor &desc = descriptorGuard.GetConstRef(); GenerateColumns(desc); if (fColumnRepresentatives.empty()) { // If we didn't get columns from the descriptor, ensure that we actually expect a field without columns diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index dd1d70955a31d..f5976f2807f18 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -520,7 +520,7 @@ std::unique_ptr ROOT::RClassField::BeforeConnectPageSource(ROO SetStagingClass(GetTypeName(), GetTypeVersion()); } else { const auto descriptorGuard = pageSource.GetSharedDescriptorGuard(); - const ROOT::RNTupleDescriptor &desc = descriptorGuard.GetRef(); + const ROOT::RNTupleDescriptor &desc = descriptorGuard.GetConstRef(); const auto &fieldDesc = desc.GetFieldDescriptor(GetOnDiskId()); if (fieldDesc.GetStructure() == ENTupleStructure::kStreamer) { @@ -608,12 +608,13 @@ std::unique_ptr ROOT::RClassField::BeforeConnectPageSource(ROO } if (nInMemoryBaseClasses != 0 && nOnDiskBaseClasses != 0 && nInMemoryBaseClasses != nOnDiskBaseClasses) { - throw RException(R__FAIL(std::string("incompatible number of base classes for field ") + GetFieldName() + ": " + - GetTypeName() + ", " + std::to_string(nInMemoryBaseClasses) + - " base classes in memory " - " vs. " + - std::to_string(nOnDiskBaseClasses) + " base classes on-disk\n" + - Internal::GetTypeTraceReport(*this, pageSource.GetSharedDescriptorGuard().GetRef()))); + throw RException( + R__FAIL(std::string("incompatible number of base classes for field ") + GetFieldName() + ": " + GetTypeName() + + ", " + std::to_string(nInMemoryBaseClasses) + + " base classes in memory " + " vs. " + + std::to_string(nOnDiskBaseClasses) + " base classes on-disk\n" + + Internal::GetTypeTraceReport(*this, pageSource.GetSharedDescriptorGuard().GetConstRef()))); } return nullptr; diff --git a/tree/ntuple/src/RNTupleMerger.cxx b/tree/ntuple/src/RNTupleMerger.cxx index c055afa131054..e666513c8dbf6 100644 --- a/tree/ntuple/src/RNTupleMerger.cxx +++ b/tree/ntuple/src/RNTupleMerger.cxx @@ -234,7 +234,7 @@ try { outSource->Attach(RNTupleSerializer::EDescriptorDeserializeMode::kForWriting); outSource->LoadAllPageLists(); auto desc = outSource->GetSharedDescriptorGuard(); - model = destination->InitFromDescriptor(desc.GetRef(), true /* copyClusters */); + model = destination->InitFromDescriptor(desc.GetConstRef(), true /* copyClusters */); } // Interface conversion @@ -997,7 +997,7 @@ class RNTupleMergeStrategyDefault final : public RNTupleMergeStrategy { { source.Attach(RNTupleSerializer::EDescriptorDeserializeMode::kForWriting); source.LoadAllPageLists(); - fSrcDescriptor = &source.GetSharedDescriptorGuard().GetRef(); + fSrcDescriptor = &source.GetSharedDescriptorGuard().GetConstRef(); if (!fDstModel) { constexpr bool copyClusters = false; diff --git a/tree/ntuple/src/RNTupleProcessor.cxx b/tree/ntuple/src/RNTupleProcessor.cxx index 08d157a253bc6..a3540cb9d024e 100644 --- a/tree/ntuple/src/RNTupleProcessor.cxx +++ b/tree/ntuple/src/RNTupleProcessor.cxx @@ -163,7 +163,7 @@ ROOT::Experimental::RNTupleSingleProcessor::CreateAndConnectField(const std::str qualifiedFieldName.find("R_rntproc_join_") == 0 ? qualifiedFieldName.substr(15) : qualifiedFieldName; auto descGuard = fPageSource->GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + const auto &desc = descGuard.GetConstRef(); const auto onDiskFieldId = desc.FindFieldId(onDiskFieldName); @@ -258,7 +258,7 @@ void ROOT::Experimental::RNTupleSingleProcessor::AddAllFieldsToEntry( { Initialize(); auto descGuard = fPageSource->GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + const auto &desc = descGuard.GetConstRef(); auto fnAddSubfields = [this, &desc, &provenance](const ROOT::RFieldDescriptor &field, auto &fn) -> void { std::string fieldName = desc.GetQualifiedFieldName(field.GetId()); if (auto prefix = provenance.Get(); !prefix.empty()) diff --git a/tree/ntuple/src/RNTupleView.cxx b/tree/ntuple/src/RNTupleView.cxx index b9694e7bf97f7..687b3c686c10a 100644 --- a/tree/ntuple/src/RNTupleView.cxx +++ b/tree/ntuple/src/RNTupleView.cxx @@ -26,7 +26,7 @@ ROOT::Internal::GetFieldRange(const ROOT::RFieldBase &field, ROOT::Internal::RPa { auto descGuard = pageSource.GetSharedDescriptorGuard(); - const auto &desc = descGuard.GetRef(); + const auto &desc = descGuard.GetConstRef(); auto parentId = desc.GetFieldDescriptor(field.GetOnDiskId()).GetParentId(); while (parentId != desc.GetFieldZeroId()) { diff --git a/tree/ntupleutil/src/RNTupleExporter.cxx b/tree/ntupleutil/src/RNTupleExporter.cxx index 4816af6ee2262..fdeab68633f90 100644 --- a/tree/ntupleutil/src/RNTupleExporter.cxx +++ b/tree/ntupleutil/src/RNTupleExporter.cxx @@ -125,7 +125,8 @@ RNTupleExporter::ExportPages(ROOT::Internal::RPageSource &source, const RPagesOp // Collect column info std::vector columnInfos; - const RAddColumnsResult addColRes = AddColumnsFromField(columnInfos, desc.GetRef(), desc->GetFieldZero(), options); + const RAddColumnsResult addColRes = + AddColumnsFromField(columnInfos, desc.GetConstRef(), desc->GetFieldZero(), options); // Collect ColumnSet for the cluster pool query ROOT::Internal::RCluster::ColumnSet_t columnSet; @@ -134,7 +135,7 @@ RNTupleExporter::ExportPages(ROOT::Internal::RPageSource &source, const RPagesOp columnSet.emplace(colInfo.fColDesc->GetPhysicalId()); } - const auto nPages = CountPages(desc.GetRef(), columnInfos); + const auto nPages = CountPages(desc.GetConstRef(), columnInfos); const bool showProgress = (options.fFlags & RPagesOptions::kShowProgressBar) != 0; res.fExportedFileNames.reserve(nPages); From b66cf2374e33af49354d5ecb5d42f5e742cf6a7a Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 8 Oct 2026 11:02:48 +0200 Subject: [PATCH 24/24] [ntuple] fix I/O counters of file backend --- tree/ntuple/inc/ROOT/RPageStorageFile.hxx | 3 ++ tree/ntuple/src/RPageStorageFile.cxx | 38 +++++++++++++---------- tree/ntuple/test/ntuple_metrics.cxx | 18 ++++++----- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RPageStorageFile.hxx b/tree/ntuple/inc/ROOT/RPageStorageFile.hxx index c0db553496be8..38b4aa7905aa9 100644 --- a/tree/ntuple/inc/ROOT/RPageStorageFile.hxx +++ b/tree/ntuple/inc/ROOT/RPageStorageFile.hxx @@ -163,6 +163,9 @@ private: std::unique_ptr PrepareSingleCluster(const ROOT::Internal::RCluster::RKey &clusterKey, std::vector &readRequests); + // Helper to calculate the distance between fLastOffset and offset and to update the skip counter accordingly + void UpdateSkipCounter(std::uint64_t offset); + protected: void LoadStructureImpl() final; ROOT::RNTupleDescriptor AttachImpl() final; diff --git a/tree/ntuple/src/RPageStorageFile.cxx b/tree/ntuple/src/RPageStorageFile.cxx index 5dfe5baa84ed0..7fdb8f4a400eb 100644 --- a/tree/ntuple/src/RPageStorageFile.cxx +++ b/tree/ntuple/src/RPageStorageFile.cxx @@ -490,26 +490,35 @@ ROOT::RNTupleDescriptor ROOT::Internal::RPageSourceFile::AttachImpl() return fDescriptorBuilder.MoveDescriptor(); } +void ROOT::Internal::RPageSourceFile::UpdateSkipCounter(std::uint64_t offset) +{ + // Track seek distance (excluding file structure reads) + if (fLastOffset == 0) + return; + + assert(fFileCounters); + const auto distance = + static_cast(std::abs(static_cast(offset) - static_cast(fLastOffset))); + fFileCounters->fSzSkip.Add(distance); +} + void ROOT::Internal::RPageSourceFile::LoadPageListImpl(const RNTupleLocator &locator, unsigned char *buffer) { + RNTupleAtomicTimer timer(fCounters->fTimeWallRead, fCounters->fTimeCpuRead); std::lock_guard lockGuard(fReaderLock); - fReader.ReadBuffer(buffer, locator.GetNBytesOnStorage(), locator.GetPosition()); + const auto offset = locator.GetPosition(); + UpdateSkipCounter(offset); + fReader.ReadBuffer(buffer, locator.GetNBytesOnStorage(), offset); + fLastOffset = offset + locator.GetNBytesOnStorage(); } void ROOT::Internal::RPageSourceFile::LoadSealedPageImpl(const RNTupleLocator &locator, RSealedPage &sealedPage) { RNTupleAtomicTimer timer(fCounters->fTimeWallRead, fCounters->fTimeCpuRead); - const auto offset = locator.GetPosition(); std::lock_guard lockGuard(fReaderLock); - // Track seek distance (excluding file structure reads) - if (fLastOffset != 0) { - R__ASSERT(fFileCounters); - const auto distance = static_cast( - std::abs(static_cast(offset) - static_cast(fLastOffset))); - fFileCounters->fSzSkip.Add(distance); - } - fReader.ReadBuffer(const_cast(sealedPage.GetBuffer()), sealedPage.GetBufferSize(), - locator.GetPosition()); + const auto offset = locator.GetPosition(); + UpdateSkipCounter(offset); + fReader.ReadBuffer(const_cast(sealedPage.GetBuffer()), sealedPage.GetBufferSize(), offset); fLastOffset = offset + sealedPage.GetBufferSize(); } @@ -687,14 +696,9 @@ ROOT::Internal::RPageSourceFile::LoadClusters(std::span clusterK std::lock_guard lockGuard(fReaderLock); // Track seek distance for each read request (excluding file structure reads) - R__ASSERT(fFileCounters); for (std::size_t i = 0; i < nBatch; ++i) { const auto offset = readRequests[iReq + i].fOffset; - if (fLastOffset != 0) { - const auto distance = static_cast(std::abs( - static_cast(offset) - static_cast(fLastOffset))); - fFileCounters->fSzSkip.Add(distance); - } + UpdateSkipCounter(offset); fLastOffset = offset + readRequests[iReq + i].fSize; } diff --git a/tree/ntuple/test/ntuple_metrics.cxx b/tree/ntuple/test/ntuple_metrics.cxx index 24bf2a8b23c53..301df6fa921e6 100644 --- a/tree/ntuple/test/ntuple_metrics.cxx +++ b/tree/ntuple/test/ntuple_metrics.cxx @@ -137,24 +137,28 @@ TEST(Metrics, IOMetrics) ntupleReader->EnableMetrics(); EXPECT_TRUE(ntupleReader->GetMetrics().IsEnabled()); - auto view = ntupleReader->GetView("ints"); - for (auto i : *ntupleReader) { - (void)view(i); - } - const auto &metrics = ntupleReader->GetMetrics(); auto *randomness = metrics.GetCounter("RNTupleReader.RPageSourceFile.randomness"); auto *sparseness = metrics.GetCounter("RNTupleReader.RPageSourceFile.sparseness"); auto *szSkip = metrics.GetCounter("RNTupleReader.RPageSourceFile.szSkip"); auto *szFile = metrics.GetCounter("RNTupleReader.RPageSourceFile.szFile"); - ASSERT_NE(randomness, nullptr); ASSERT_NE(sparseness, nullptr); ASSERT_NE(szSkip, nullptr); ASSERT_NE(szFile, nullptr); - EXPECT_GE(szSkip->GetValueAsInt(), 0); + EXPECT_EQ(szSkip->GetValueAsInt(), 0); EXPECT_GT(szFile->GetValueAsInt(), 0); + + auto view = ntupleReader->GetView("ints"); + const auto pageListSkip = szSkip->GetValueAsInt(); + EXPECT_GE(pageListSkip, 0); + + for (auto i : *ntupleReader) { + (void)view(i); + } + + EXPECT_GE(szSkip->GetValueAsInt(), pageListSkip); } }