From bf8a5301499b0b473b9d3b95518ae107d7e3bb70 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 28 Jul 2026 21:17:04 +0000 Subject: [PATCH] [tree] Fix bulk read inflating the reported branch size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling `GetBulkRead().GetBulkEntries()/GetEntriesSerialized()` on a branch increased its reported Total Size by 4 bytes on every call, so the size was not invariant with respect to the API used to read the branch. Root cause: a bulk read loads a basket into fBaskets via `TObjArray::AddAt` (`TBranch::GetBasketImpl`), which raises the array's fLast. The bulk-IO path then disassociates that basket from the array with a plain slot assignment, `fBaskets[fReadBasket] = nullptr`. That is not the inverse of `AddAt:` the non-const `TObjArray::operator[]` does `fLast = std::max(j, GetAbsLast()`), so writing the null actually pins `fLast` at `fReadBasket` rather than lowering it. The array is then streamed (by `GetTotalSize()`/`Print()`, and on a subsequent `TTree::Write()`) with one extra trailing slot, and `TObjArray::Streamer` emits an additional 4-byte null-pointer marker for it. Fix: drop the basket with `fBaskets.RemoveAt(fReadBasket)` at the three bulk-IO disassociation sites (GetBasketAndFirst, GetBulkEntries, GetEntriesSerialized). RemoveAt is the proper inverse of the AddAt done when the basket was loaded: it clears the slot and walks fLast back down past trailing nulls, matching what DropBaskets already does. Notes for review: - Fix location. The alternative is to make TObjArray::Streamer / GetTotalSize tolerant of trailing null slots. Fixing the read path was chosen instead so that a bulk read leaves no observable state change at all, which is what the issue asks for, and it is narrower than teaching the streamer to compact. - Performance. Bulk IO is a hot path, but this adds no meaningful cost: the previous operator[] assignment already acquired gCoreMutex (a conditional read guard, a no-op unless thread-safety is enabled) and already called Changed(). The only delta is read-guard -> write-guard. - Scope. Other sites null a slot the same inflating way and share the latent bug (the fBaskets[i] = nullptr in the flush path, and the fBaskets.AddAt(nullptr, idx) calls in the basket-unload paths). They are intentionally left untouched here: they are unrelated code paths not exercised by this issue, and are better addressed separately. - RemoveAt is a no-op on an already-null slot, so there is no double-remove hazard when GetBulkEntries runs after GetBasketAndFirst cleared the slot. The disassociated basket is not leaked; it is still parked in fExtraBasket. Adds a regression test (`BulkApiTest.SizeInvariantAfterBulkRead`) asserting `GetTotalSize()` is invariant across a bulk read; it fails without the fix. Closes #8961 🤖 Done with the help of AI. --- tree/tree/src/TBranch.cxx | 15 +++++++++++--- tree/tree/test/BulkApi.cxx | 41 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/tree/tree/src/TBranch.cxx b/tree/tree/src/TBranch.cxx index 1f6bd5a1dafb6..a49a37c81abb8 100644 --- a/tree/tree/src/TBranch.cxx +++ b/tree/tree/src/TBranch.cxx @@ -1409,8 +1409,11 @@ Int_t TBranch::GetBasketAndFirst(TBasket *&basket, Long64_t &first, // Disassociate basket from memory buffer for bulk IO // When the user provides a memory buffer (i.e., for bulk IO), we should // make sure to drop all references to that buffer in the TTree afterward. + // Use RemoveAt (rather than a plain slot assignment) so that the array's + // fLast bookkeeping is updated; otherwise the branch would be streamed with + // an extra (empty) basket slot, spuriously increasing its reported size. fCurrentBasket = nullptr; - fBaskets[fReadBasket] = nullptr; + fBaskets.RemoveAt(fReadBasket); } else { fCurrentBasket = basket; } @@ -1515,7 +1518,10 @@ Int_t TBranch::GetBulkEntries(Long64_t entry, TBuffer &user_buf) user_buf.SetBuffer(buf->Buffer(), buf->BufferSize()); buf->ResetBit(TBufferIO::kIsOwner); fCurrentBasket = nullptr; - fBaskets[fReadBasket] = nullptr; + // Use RemoveAt so the array's fLast bookkeeping is updated; a plain slot + // assignment would leave the branch streaming an extra (empty) basket slot, + // spuriously increasing its reported size (see issue #8961). + fBaskets.RemoveAt(fReadBasket); } else { // This is the only copy, we can't return it as is to the user, just make a copy. if (user_buf.BufferSize() < buf->BufferSize()) { @@ -1633,7 +1639,10 @@ Int_t TBranch::GetEntriesSerialized(Long64_t entry, TBuffer &user_buf, TBuffer * user_buf.SetBuffer(buf->Buffer(), buf->BufferSize()); buf->ResetBit(TBufferIO::kIsOwner); fCurrentBasket = nullptr; - fBaskets[fReadBasket] = nullptr; + // Use RemoveAt so the array's fLast bookkeeping is updated; a plain slot + // assignment would leave the branch streaming an extra (empty) basket slot, + // spuriously increasing its reported size (see issue #8961). + fBaskets.RemoveAt(fReadBasket); } else { // This is the only copy, we can't return it as is to the user, just make a copy. if (user_buf.BufferSize() < buf->BufferSize()) { diff --git a/tree/tree/test/BulkApi.cxx b/tree/tree/test/BulkApi.cxx index 2bd650dd5226c..41363134f4941 100644 --- a/tree/tree/test/BulkApi.cxx +++ b/tree/tree/test/BulkApi.cxx @@ -1,4 +1,5 @@ #include +#include #include "Bytes.h" #include "TBranch.h" @@ -255,3 +256,43 @@ TEST_F(BulkApiTest, BulkInMem) break; } } + +// https://github.com/root-project/root/issues/8961 +// Reading a branch with the bulk API must not change its reported size: the bulk +// path temporarily loads a basket into the fBaskets array and must remove it +// cleanly afterwards, otherwise the branch streams an extra (empty) basket slot +// and its total size grows by 4 bytes on every read. +TEST_F(BulkApiTest, SizeInvariantAfterBulkRead) +{ + const std::string fileName = "BulkApiSizeInvariant.root"; + { + // Heap-allocated to avoid the intermittent failures that stack-allocated + // TFile/TTree have caused in the past. The TTree is owned by the file. + std::unique_ptr f(TFile::Open(fileName.c_str(), "recreate")); + f->SetCompressionLevel(0); + auto t = new TTree("T", "T"); + int x = 0; + t->Branch("x", &x, 8000, 0); // small basket size -> many baskets + for (int i = 0; i < 100000; ++i) { + x = i; + t->Fill(); + } + f->Write(); + } + + std::unique_ptr f(TFile::Open(fileName.c_str())); + ASSERT_NE(nullptr, f); + auto t = f->Get("T"); // owned by the file + ASSERT_NE(nullptr, t); + TBranch *b = t->GetBranch("x"); + ASSERT_NE(nullptr, b); + + const Long64_t sizeBefore = b->GetTotalSize(); + + TBufferFile buf(TBuffer::EMode::kWrite, 32 * 1024); + auto s = b->GetBulkRead().GetBulkEntries(0, buf); + ASSERT_GT(s, 0) << "Did not read any entry with the bulk API."; + + const Long64_t sizeAfter = b->GetTotalSize(); + EXPECT_EQ(sizeBefore, sizeAfter) << "The branch size must be invariant from the API used to read it."; +}