Fix TFile gap coalescing

io/io/inc/TFree.h

@@ -23,14 +23,16 @@
 
 #include "TObject.h"
 
-
 class TFree : public TObject {
-
 protected:
    Long64_t        fFirst;            ///<First free word of segment
    Long64_t        fLast;             ///<Last free word of segment
 
 public:
+   /// Prevent gap coalescing from exceeding 2 GB, since the gap size is stored as a signed 4 bytes integer.
+   /// The true limit is 2 GiB but for historical reasons (previous implementation of TFree) we stick to the 2GB limit.
+   static constexpr Int_t kMaxGapSize = 2000 * 1000 * 1000;
+
    TFree();
    TFree(TList *lfree, Long64_t first, Long64_t last);
    ~TFree() override;

io/io/src/TFile.cxx

@@ -1496,27 +1496,59 @@ Bool_t TFile::IsOpen() const
 
 void TFile::MakeFree(Long64_t first, Long64_t last)
 {
-   TFree *f1      = (TFree*)fFree->First();
-   if (!f1) return;
-   TFree *newfree = f1->AddFree(fFree,first,last);
-   if(!newfree) return;
+   assert(first > 0 && last > first && last < fEND);
+
+   TFree *f1 = static_cast<TFree *>(fFree->First());
+   assert(f1); // There must always be at least the virtual free segment at the end of the file
+
+   TFree *newfree = f1->AddFree(fFree, first, last);
+   assert(newfree); // AddFree always succeeds
+
    Long64_t nfirst = newfree->GetFirst();
    Long64_t nlast  = newfree->GetLast();
-   Long64_t nbytesl= nlast-nfirst+1;
-   if (nbytesl > 2000000000) nbytesl = 2000000000;
-   Int_t nbytes    = -Int_t (nbytesl);
-   char buffer[sizeof(Int_t)];
-   char *pbuffer = buffer;
-   tobuf(pbuffer, nbytes);
-   if (last == fEND-1) fEND = nfirst;
-   Seek(nfirst);
-   // We could not update the meta data for this block on the file.
-   // This is not fatal as this only means that we won't get it 'right'
-   // if we ever need to Recover the file before the block is actually
-   // (attempted to be reused.
-   // coverity[unchecked_value]
-   WriteBuffer(buffer, sizeof(buffer));
-   if (fMustFlush) Flush();
+
+   // The new free segment may close a series of consecutive free segments at the end of the file.
+   // These segments need to be merged so that the last free segment is [fEND + 1 .. max file size].
+   auto tail = static_cast<TFree *>(fFree->Last());
+   while (tail != fFree->First()) {
+      // Starting from the last segment, merge in previous segments if they are adjacent
+      auto prev = static_cast<TFree *>(fFree->Before(tail));
+      if (prev->GetLast() + 1 < tail->GetFirst())
+         break;
+      assert(prev->GetLast() + 1 == tail->GetFirst());
+      tail->SetFirst(prev->GetFirst());
+      fFree->Remove(prev);
+      delete prev;
+   }
+   if (tail->GetFirst() <= nfirst) {
+      nfirst = tail->GetFirst();
+      nlast = tail->GetLast();
+   }
+
+   // The file will get smaller if a free segment is added at the end. In this case, we are done and we don't need
+   // to write the free segment size past the new end of the file (besides, the length of this "virtual" free
+   // segment may anyway be larger than TFree::kMaxGapSize).
+   if (last == fEND - 1) {
+      fEND = nfirst;
+   } else {
+      Long64_t nbytesl = nlast - nfirst + 1;
+      assert(nbytesl <= TFree::kMaxGapSize);
+
+      Int_t nbytes = -static_cast<Int_t>(nbytesl);
+      char buffer[sizeof(Int_t)];
+      char *pbuffer = buffer;
+      tobuf(pbuffer, nbytes);
+
+      Seek(nfirst);
+      if (WriteBuffer(buffer, sizeof(buffer)) != 0) {
+         // Not fatal, this only means that we won't get it 'right'
+         // if we ever need to Recover the file before the block is actually
+         // (attempted to be reused.
+         Warning("TFile::MakeFree()", "failed to write free segment header");
+      }
+      if (fMustFlush)
+         Flush();
+   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -2117,7 +2149,11 @@ Int_t TFile::Recover()
 
    fEND = Long64_t(size);
 
-   if (fWritable && !fFree) fFree  = new TList;
+   if (fWritable) {
+      if (!fFree)
+         fFree = new TList();
+      fFree->Delete();
+   }
 
    Int_t nrecov = 0;
    nwheader = 1024;
@@ -2190,12 +2226,7 @@ Int_t TFile::Recover()
    if (fWritable) {
       Long64_t max_file_size = Long64_t(kStartBigFile);
       if (max_file_size < fEND) max_file_size = fEND+1000000000;
-      TFree *last = (TFree*)fFree->Last();
-      if (last) {
-         last->AddFree(fFree,fEND,max_file_size);
-      } else {
-         new TFree(fFree,fEND,max_file_size);
-      }
+      new TFree(fFree, fEND, max_file_size);
       if (nrecov) Write();
    }
    return nrecov;

io/io/src/TFree.cxx

@@ -61,25 +61,38 @@ TFree::TFree(TList *lfree, Long64_t first, Long64_t last)
 ///   - if first just follows an existing free segment AND last just precedes
 ///     an existing free segment, these two segments are merged into
 ///     one single segment.
+///   - Do not join free segments, however, if the combined resulting segment
+///     would be larger than 2GB.
 ///
 
 TFree *TFree::AddFree(TList *lfree, Long64_t first, Long64_t last)
 {
+   assert(first < last);
+
    TFree *idcur = this;
-   while (idcur) {
+   do {
       Long64_t curfirst = idcur->GetFirst();
       Long64_t curlast  = idcur->GetLast();
-      if (curlast == first-1) {
+      // We can't have overlapping free segments
+      assert((first < curfirst && last < curfirst) || first > curlast);
+
+      if ((curlast == first - 1) && (last - curfirst < kMaxGapSize)) {
          idcur->SetLast(last);
-         TFree *idnext = (TFree*)lfree->After(idcur);
-         if (idnext == 0) return idcur;
-         if (idnext->GetFirst() > last+1) return idcur;
-         idcur->SetLast( idnext->GetLast() );
+         // Merged new segment with previous one; is there a next segment?
+         TFree *idnext = static_cast<TFree *>(lfree->After(idcur));
+         if (idnext == nullptr)
+            return idcur;
+
+         // Continue only if the next segment is adjacent to the newly merged one (and not too big)
+         if ((idnext->GetFirst() > last + 1) || (idnext->GetLast() - curfirst >= kMaxGapSize))
+            return idcur;
+
+         idcur->SetLast(idnext->GetLast());
          lfree->Remove(idnext);
          delete idnext;
          return idcur;
       }
-      if (curfirst == last+1) {
+      if ((curfirst == last + 1) && (curlast - first < kMaxGapSize)) {
          idcur->SetFirst(first);
          return idcur;
       }
@@ -91,8 +104,11 @@ TFree *TFree::AddFree(TList *lfree, Long64_t first, Long64_t last)
          return newfree;
       }
       idcur = (TFree*)lfree->After(idcur);
-   }
-   return 0;
+   } while (idcur);
+
+   // never here
+   assert(false);
+   return nullptr;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -186,4 +202,3 @@ Int_t TFree::Sizeof() const
    if (fLast > TFile::kStartBigFile) return 18;
    else                              return 10;
 }
-

io/io/test/TFileTests.cxx

@@ -17,6 +17,8 @@
 #include "TSystem.h"
 #include "TEnv.h" // gEnv
 
+#include <cstdio>
+
 TEST(TFile, WriteObjectTObject)
 {
    auto filename{"tfile_writeobject_tobject.root"};
@@ -332,3 +334,134 @@ TEST(TFile, UUID)
    TMemFile f("uuidtest.root", "RECREATE");
    EXPECT_EQ('4', f.GetUUID().AsString()[14]);
 }
+
+TEST(TFile, DeleteKey)
+{
+   struct FileRaii {
+      std::string fFilename;
+      FileRaii(std::string_view fname) : fFilename(fname) {}
+      ~FileRaii() { gSystem->Unlink(fFilename.c_str()); }
+   } fileGuard("tfile_test_delete_keys.root");
+
+   auto fnCountGaps = [](const std::string &fileName) {
+      auto f = std::unique_ptr<TFile>(TFile::Open(fileName.c_str()));
+      std::uint64_t nGaps = 0;
+      for (const auto &k : f->WalkTKeys()) {
+         if (k.fType == ROOT::Detail::TKeyMapNode::kGap)
+            nGaps++;
+      }
+      return nGaps;
+   };
+
+   auto f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "RECREATE"));
+   f->SetCompressionSettings(0);
+   f->Write();
+   f->Close();
+
+   // The empty file should have no gaps. Note that gaps are created temporarily when certain keys are overwritten.
+   EXPECT_EQ(0, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   std::vector<char> v;
+   f->WriteObject(&v, "va0");
+   f->WriteObject(&v, "va1");
+   f->WriteObject(&v, "va2");
+   f->Write();
+   f->Close();
+   // 2 gaps: new (larger) keys list and free list are written
+   EXPECT_EQ(2, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   f->Delete("va1;*"); // should create small gap that cannot be merged, trapped between v0 and v2
+   f->Write();
+   f->Close();
+
+   EXPECT_EQ(3, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   f->Delete("va2;*"); // gaps at the tail should merge
+   f->Write();
+   f->Close();
+
+   EXPECT_EQ(2, fnCountGaps(fileGuard.fFilename));
+
+   // The following tests run out of memory on 32bit platforms
+   if (sizeof(std::size_t) == 4) {
+      printf("Skipping test partially on 32bit platform.\n");
+      return;
+   }
+
+   v.resize(1000 * 1000 * 1000 - 100, 'x'); // almost 1GB
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   f->WriteObject(&v, "vb0");
+   f->WriteObject(&v, "vb1");
+   v.resize(1000 * 1000); // truncate next objects to 1MB
+   f->WriteObject(&v, "vc0");
+   f->WriteObject(&v, "vc1");
+   f->WriteObject(&v, "vc2");
+   f->WriteObject(&v, "vc3");
+   f->Write();
+   EXPECT_GT(f->GetEND(), TFile::kStartBigFile);
+   f->Close();
+
+   // New keys list, hence 3 gaps
+   EXPECT_EQ(3, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   f->Delete("vb0;*"); //  |
+   f->Delete("vb1;*"); //  |---> First merged gap
+   f->Delete("vc0;*"); //  |
+   f->Delete("vc1;*"); //  |---> Second merged gap
+   f->Write();
+   f->Close();
+
+   // Two merged gaps, the new keys list fits into either of them
+   EXPECT_EQ(4, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   // Delete the remaining data at the tail of the file in reverse order
+   f->Delete("vc2;*");
+   f->Delete("vc3;*");
+   f->Write();
+   // Back to small file
+   EXPECT_LT(f->GetEND(), TFile::kStartBigFile);
+   f->Close();
+
+   // Only the original 2 gaps from the first keys list and free list overwrite
+   EXPECT_EQ(2, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   v.resize(700 * 1000 * 1000); // construct objects such that 3 consecutive gaps surpass 2GB (but not 2)
+   f->WriteObject(&v, "vd0");
+   f->WriteObject(&v, "vd1");
+   f->WriteObject(&v, "vd2");
+   f->WriteObject(&v, "vd3");
+   f->WriteObject(&v, "vd4");
+   f->Write();
+   f->Close();
+
+   // New keys list --> 3 gaps
+   EXPECT_EQ(3, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   f->Delete("vd1;*");
+   f->Delete("vd3;*");
+   f->Write();
+   f->Close();
+
+   // Nothing mergable, 2 more gaps
+   EXPECT_EQ(5, fnCountGaps(fileGuard.fFilename));
+
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str(), "UPDATE"));
+   auto theEnd = f->GetEND();
+   f->Delete("vd2;*");
+   f->Write();
+   f->Close();
+
+   // We can only merge the gaps of v1 and v2, not all three (vd1, vd2, vd3) due to the gap size
+   EXPECT_EQ(5, fnCountGaps(fileGuard.fFilename));
+
+   // Ensure that the file's tail is still intact
+   f = std::unique_ptr<TFile>(TFile::Open(fileGuard.fFilename.c_str()));
+   EXPECT_EQ(f->GetEND(), theEnd);
+}