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."; +}