diff --git a/io/io/inc/TFree.h b/io/io/inc/TFree.h index 0acaad50aa1e5..2c4553e22ea4c 100644 --- a/io/io/inc/TFree.h +++ b/io/io/inc/TFree.h @@ -23,14 +23,16 @@ #include "TObject.h" - class TFree : public TObject { - protected: Long64_t fFirst; ///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(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(fFree->Last()); + while (tail != fFree->First()) { + // Starting from the last segment, merge in previous segments if they are adjacent + auto prev = static_cast(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(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; diff --git a/io/io/src/TFree.cxx b/io/io/src/TFree.cxx index 5a06ac2497ada..d4eb14cb40e6a 100644 --- a/io/io/src/TFree.cxx +++ b/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(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; } - diff --git a/io/io/test/TFileTests.cxx b/io/io/test/TFileTests.cxx index 5ef2ed684feee..b0ef02db191f1 100644 --- a/io/io/test/TFileTests.cxx +++ b/io/io/test/TFileTests.cxx @@ -17,6 +17,8 @@ #include "TSystem.h" #include "TEnv.h" // gEnv +#include + 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::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::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::Open(fileGuard.fFilename.c_str(), "UPDATE")); + std::vector 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::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::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::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::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::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::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::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::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::Open(fileGuard.fFilename.c_str())); + EXPECT_EQ(f->GetEND(), theEnd); +}