[tree] Fix bulk read inflating the reported branch size

[guitargeek]

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, then disassociates it again 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, for which TObjArray::Streamer emits a 4-byte null marker.

Fix: route the three bulk-IO disassociation sites (GetBasketAndFirst, GetBulkEntries, GetEntriesSerialized) through a new private helper, TBranch::DisassociateBulkBasket(), which clears the slot and brings both pieces of bookkeeping back in sync: fLast, and fNBaskets (see below).

Adds a regression test (BulkApiTest.SizeInvariantAfterBulkRead) reading a branch to the end with the bulk API, asserting after every call that GetTotalSize() is unchanged and that no basket was left in the array. Without the fix it grows by exactly 4 bytes per call.

Notes for review:

• Performance, and why not TObjArray::RemoveAt. Dropping the basket with fBaskets.RemoveAt(fReadBasket) is the obvious way to keep fLast correct, but it is quadratic here: a bulk read holds exactly one basket, at an index that grows by one per call, so RemoveAt always hits its i == fLast case and its backward scan (do { fLast--; } while (fLast >= 0 && !fCont[fLast]);) finds every lower slot empty and walks all the way down to -1.

  Timing just that access pattern, a clean factor 4 per doubling (root -b -q 'basketScanBench.C(100000)'):

  /// Cost of the fLast bookkeeping in the access pattern a bulk read produces:
  /// the array holds a single basket at an index that grows by one per read.
  void basketScanBench(Int_t nbaskets = 100000)
  {
     TObjString basket("basket");
     TStopwatch sw;
  
     TObjArray withRemoveAt(nbaskets);
     sw.Start();
     for (Int_t i = 0; i < nbaskets; ++i) {
        withRemoveAt.AddAt(&basket, i);
        withRemoveAt.RemoveAt(i);
     }
     printf("%7d baskets  RemoveAt    %8.4f s\n", nbaskets, sw.RealTime());
  
     TObjArray withSetLast(nbaskets);
     sw.Start();
     for (Int_t i = 0; i < nbaskets; ++i) {
        withSetLast.AddAt(&basket, i);
        withSetLast.AddAt(nullptr, i);
        withSetLast.SetLast(-1);
     }
     printf("%7d baskets  SetLast(-1) %8.4f s\n", nbaskets, sw.RealTime());
  }

  baskets   RemoveAt   SetLast(-1)
  25000     0.0777 s     0.0005 s
  50000     0.3272 s     0.0009 s
  100000     1.2910 s     0.0018 s
  200000     5.0699 s     0.0035 s
  

  End to end on a branch with 107298 baskets (root -b -q 'bulkReadBench.C(25000000, 1000)'):

  /// Time a full bulk read of a branch with many baskets.
  void bulkReadBench(Long64_t nentries = 25000000, Int_t basketBytes = 1000)
  {
     const TString fname = TString::Format("bulkReadBench_%lld_%d.root", nentries, basketBytes);
  
     if (gSystem->AccessPathName(fname)) { // reuse the file across builds
        std::unique_ptr<TFile> f{TFile::Open(fname, "recreate")};
        f->SetCompressionLevel(0);
        auto t = new TTree("T", "T");
        t->SetAutoFlush(0); // keep the small baskets, OptimizeBaskets() would grow them
        int x = 0;
        t->Branch("x", &x, basketBytes, 0);
        for (Long64_t i = 0; i < nentries; ++i) {
           x = i;
           t->Fill();
        }
        f->Write();
     }
  
     std::unique_ptr<TFile> f{TFile::Open(fname)};
     auto b = f->Get<TTree>("T")->GetBranch("x");
  
     TBufferFile buf(TBuffer::kWrite, 32 * 1024);
     TStopwatch sw;
     sw.Start();
     for (Long64_t entry = 0; entry < nentries;) {
        const auto n = b->GetBulkRead().GetBulkEntries(entry, buf);
        if (n <= 0) {
           printf("bulk read failed at entry %lld\n", entry);
           return;
        }
        entry += n;
     }
     printf("%7d baskets  bulk read   %8.4f s\n", b->GetWriteBasket() + 1, sw.RealTime());
  }

  master (plain slot assignment)   0.15 s
  with TObjArray::RemoveAt         1.70 s   <- 11x slower
  this patch                       0.15 s

  The scan would be 80% of the whole bulk read, dwarfing the actual IO. DisassociateBulkBasket instead answers the question without searching: when the array is left empty, the new fLast is simply -1. That is the fast case DropBaskets already uses, guard included. RemoveAt is kept for when other baskets are still resident (cluster prefetching), where the lookup is real work.

• fNBaskets. The fast-path guard needs it accurate, and it was not: the bulk-IO paths increment it in GetBasketImpl but never decremented it when dropping the basket, so it drifted upwards on every bulk read, silently mis-steering GetFreshBasket's fNBaskets == 1 basket reuse and DropBaskets's fast/slow choice. The helper now decrements it, a fix in its own right.

• Scope. Fixing the read path was preferred over making TObjArray::Streamer / GetTotalSize tolerant of trailing nulls, so that a bulk read leaves no observable state change at all. Other sites null a slot the same inflating way (the flush path, the basket-unload paths) and share the latent bug, but they are unrelated code paths not exercised by this issue and are better addressed separately.

• Only one of the two disassociation sites fires per bulk read: if the basket had to be loaded, GetBasketAndFirst clears it and GetBulkEntries then sees &user_buf == buf; otherwise only GetBulkEntries clears it. The helper is a no-op on an already-cleared slot anyway. The basket is not leaked, it is parked in fExtraBasket.

Closes #8961

🤖 Done with the help of AI.

[github-actions]

Test Results

    23 files      23 suites   3d 18h 24m 8s ⏱️
 3 853 tests  3 850 ✅ 0 💤 3 ❌
79 408 runs  79 404 ✅ 1 💤 3 ❌

For more details on these failures, see this check.

Results for commit 89ade40.

♻️ This comment has been updated with latest results.

[pcanal]

The Performance analysis seems incomplete. RemoveAt adds a scan through the array to find the real last item (when the item removed is the last). For very large TTree this could be a noticeable scan.