diff --git a/roottest/root/tree/stl/CMakeLists.txt b/roottest/root/tree/stl/CMakeLists.txt index 3f2910af58e4e..1e6b80aa53ae3 100644 --- a/roottest/root/tree/stl/CMakeLists.txt +++ b/roottest/root/tree/stl/CMakeLists.txt @@ -45,3 +45,10 @@ ROOTTEST_ADD_TEST(simple OUTREF simple.ref FIXTURES_REQUIRED root-tree-stl-copylogon-fixture root-tree-stl-runsimple-fixture) +ROOTTEST_COMPILE_MACRO(execPartialMap.cxx + FIXTURES_SETUP root-tree-stl-execPartialMap-fixture) + +ROOTTEST_ADD_TEST(PartialMap + MACRO execPartialMap.cxx+ + OUTREF execPartialMap.ref + FIXTURES_REQUIRED root-tree-stl-execPartialMap-fixture) diff --git a/roottest/root/tree/stl/execPartialMap.cxx b/roottest/root/tree/stl/execPartialMap.cxx new file mode 100644 index 0000000000000..1d055aaaf22ac --- /dev/null +++ b/roottest/root/tree/stl/execPartialMap.cxx @@ -0,0 +1,194 @@ +// This is testing the issue reported at https://github.com/root-project/root/issues/19963 + +#include +#include +#include +#include + +#include "Rtypes.h" +#include "TBuffer.h" + +class AddressKey { +public: + int id; + std::string name; + + AddressKey(int i = 0, const std::string &n = "") : id(i), name(n) {} + + // Comparison operator for std::map + bool operator<(const AddressKey &other) const { return (id < other.id) || (id == other.id && name < other.name); } + + static int fgStreamerCalled; + + ClassDef(AddressKey, 1) // Example class with custom Streamer +}; + +inline void AddressKey::Streamer(TBuffer &b) +{ + ++fgStreamerCalled; + // Default StreamerInfo-based ROOT I/O for a class with one int member + // This is the standard pattern for custom Streamer functions + if (b.IsReading()) { + b.ReadClassBuffer(AddressKey::Class(), this); + } else { + b.WriteClassBuffer(AddressKey::Class(), this); + } +} + +class AddressValue { +public: + int fData = -1; + + static int fgStreamerCalled; + + ClassDef(AddressValue, 1) // Example class with custom Streamer +}; + +inline void AddressValue::Streamer(TBuffer &b) +{ + ++fgStreamerCalled; + // Default StreamerInfo-based ROOT I/O for a class with one int member + // This is the standard pattern for custom Streamer functions + if (b.IsReading()) { + b.ReadClassBuffer(AddressValue::Class(), this); + } else { + b.WriteClassBuffer(AddressValue::Class(), this); + } +} + +// Static member definitions +int AddressKey::fgStreamerCalled = 0; +int AddressValue::fgStreamerCalled = 0; + +#ifdef __ROOTCLING__ +#pragma link C++ class AddressKey-; +#pragma link C++ class AddressValue-; +#pragma link C++ class std::map+; +#pragma link C++ class std::pair+; +#pragma link C++ class std::vector >+; +#endif + +#include "TFile.h" +#include "TTree.h" +#include "TBranch.h" + +TTree *CreateTree() +{ + TTree *tree = new TTree("tree", "A tree with AddressPrinter objects"); + std::map vec; + tree->Branch("map", &vec); + + // Fill with one element + vec[0] = {AddressValue()}; + tree->Fill(); + + // Fill with two elements + vec[1] = {AddressValue()}; + tree->Fill(); + + // Fill with one element again + vec[2] = {AddressValue()}; + tree->Fill(); + + tree->ResetBranchAddresses(); + return tree; +} + +template +struct References; + +template <> +struct References> { + static constexpr std::array KeyCount = {1, 3, 6}; + static constexpr std::array ValueCount = {1, 3, 6}; +}; + +template <> +struct References>> { + static constexpr std::array KeyCount = {0, 0, 0}; + static constexpr std::array ValueCount = {1, 3, 6}; +}; + +template > +bool ReadTree(TTree *tree) +{ + constexpr std::array expectedReadKeyCountValues = References::KeyCount; + constexpr std::array expectedReadValueCountValues = References::ValueCount; + + AddressKey::fgStreamerCalled = 0; + AddressValue::fgStreamerCalled = 0; + + bool result = true; + + // std::vector *map = nullptr; + // std::map *m = nullptr; + Collection *m = nullptr; + tree->SetBranchAddress("map", &m); + + ULong64_t nentries = tree->GetEntries(); + for (ULong64_t i = 0; i < nentries; ++i) { + std::cout << "Entry " << i << ":\n"; + // tree->GetEntry(i); + tree->GetBranch("map")->TBranch::GetEntry(i); + tree->GetBranch("map.second")->GetEntry(i); + if (!m) { + std::cerr << "ERROR: nullptr branch\n"; + continue; + } + std::cout << "AddressKey::Streamer called " << AddressKey::fgStreamerCalled << " times\n"; + std::cout << "AddressValue::Streamer called " << AddressValue::fgStreamerCalled << " times\n"; + int expectedKeyCount = (i < expectedReadKeyCountValues.size()) ? expectedReadKeyCountValues[i] : -1; + if (AddressKey::fgStreamerCalled != expectedKeyCount) { + std::cerr << "ERROR: AddressKey::fgStreamerCalled=" << AddressKey::fgStreamerCalled + << ", expected=" << expectedKeyCount << std::endl; + result = false; + } + int expectedValueCount = (i < expectedReadValueCountValues.size()) ? expectedReadValueCountValues[i] : -1; + if (AddressValue::fgStreamerCalled != expectedValueCount) { + std::cerr << "ERROR: AddressValue::fgStreamerCalled=" << AddressValue::fgStreamerCalled + << ", expected=" << expectedValueCount << std::endl; + result = false; + } + } + tree->ResetBranchAddresses(); + delete m; + return result; +} + +int readwrite() +{ + std::cout << "Creating tree\n"; + std::unique_ptr f(TFile::Open("objects.root", "RECREATE")); + + TTree *tree = CreateTree(); + f->Write(); + std::cout << "Reading tree\n"; + auto result = ReadTree(tree); + std::cout << "Deleting tree\n"; + delete tree; + std::cout << "done\n"; + return result ? 0 : 1; +} + +template +int justread() +{ + std::cout << "Open file\n"; + std::unique_ptr f(TFile::Open("objects.root", "READ")); + auto tree = f->Get("tree"); + std::cout << "Reading tree\n"; + auto result = ReadTree(tree); + std::cout << "Deleting tree\n"; + delete tree; + std::cout << "done\n"; + return result ? 0 : 2; +} + +int execPartialMap() +{ + int ret1 = readwrite(); + AddressKey::fgStreamerCalled = 0; + AddressValue::fgStreamerCalled = 0; + int ret2 = justread>>(); + return ret1 + ret2; +} diff --git a/roottest/root/tree/stl/execPartialMap.ref b/roottest/root/tree/stl/execPartialMap.ref new file mode 100644 index 0000000000000..847f75137d1a5 --- /dev/null +++ b/roottest/root/tree/stl/execPartialMap.ref @@ -0,0 +1,30 @@ +Automatic building of dictionaries now off + +Processing /uscms_data/d2/pcanal/eaf_root_working/code/quick-devel/roottest/root/tree/stl/execPartialMap.cxx+... +Creating tree +Reading tree +Entry 0: +AddressKey::Streamer called 1 times +AddressValue::Streamer called 1 times +Entry 1: +AddressKey::Streamer called 3 times +AddressValue::Streamer called 3 times +Entry 2: +AddressKey::Streamer called 6 times +AddressValue::Streamer called 6 times +Deleting tree +done +Open file +Reading tree +Entry 0: +AddressKey::Streamer called 0 times +AddressValue::Streamer called 1 times +Entry 1: +AddressKey::Streamer called 0 times +AddressValue::Streamer called 3 times +Entry 2: +AddressKey::Streamer called 0 times +AddressValue::Streamer called 6 times +Deleting tree +done +(int) 0 diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 45031645e7402..76474dcb4862d 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -77,6 +77,40 @@ namespace { if (fOnfileObject) fBuffer.PopDataCache(); } }; + //////////////////////////////////////////////////////////////////////////////// + /// Check if a collection proxy represents an associative collection (e.g., std::map, std::set) + /// rather than a sequential collection (e.g., std::vector, std::list). + /// Both the version based on the fSTLtype integer and the one based on the TVirtualCollectionProxy + /// will return the same result about the 'currently' in memory collection attached to the branch. + /// The main difference is that the fSTLtype can be used without checking whether + /// fCollProxy is set or not but might (or might not) be a tad bit slower. + bool IsAssociativeContainer(Int_t stltype) + { + switch (stltype) { + case ROOT::kSTLset: + case ROOT::kSTLmultiset: + case ROOT::kSTLunorderedset: + case ROOT::kSTLunorderedmultiset: + case ROOT::kSTLmap: + case ROOT::kSTLmultimap: + case ROOT::kSTLunorderedmap: + case ROOT::kSTLunorderedmultimap: + return true; + default: + return false; + } + } + bool IsAssociativeContainer(const TVirtualCollectionProxy &proxy) + { + return proxy.GetProperties() & TVirtualCollectionProxy::kIsAssociative; + } + + void RecursiveResetReadEntry(TBranch *br) + { + br->ResetReadEntry(); + for (auto sub : *br->GetListOfBranches()) + RecursiveResetReadEntry(static_cast(sub)); + } } //////////////////////////////////////////////////////////////////////////////// @@ -1454,7 +1488,7 @@ void TBranchElement::FillLeavesCollection(TBuffer& b) //NOTE: this does not work for not vectors since the CreateIterators expects a TGenCollectionProxy::TStaging as its argument! //NOTE: and those not work in general yet, since the TStaging object is neither created nor passed. // We need to review how to avoid the need for a TStaging during the writing. - if (proxy->GetProperties() & TVirtualCollectionProxy::kIsAssociative) { + if (IsAssociativeContainer(*proxy)) { fWriteIterators->CreateIterators(fObject, proxy); } else { fIterators->CreateIterators(fObject, proxy); @@ -2672,6 +2706,7 @@ TClass* TBranchElement::GetCurrentClass() Int_t TBranchElement::GetEntry(Long64_t entry, Int_t getall) { // Remember which entry we are reading. + auto prevEntry = fReadEntry; fReadEntry = entry; // If our tree has a branch ref, make it remember the entry and @@ -2711,27 +2746,17 @@ Int_t TBranchElement::GetEntry(Long64_t entry, Int_t getall) } nbytes += nb; } - switch(fSTLtype) { - case ROOT::kSTLset: - case ROOT::kSTLmultiset: - case ROOT::kSTLunorderedset: - case ROOT::kSTLunorderedmultiset: - case ROOT::kSTLmap: - case ROOT::kSTLmultimap: - case ROOT::kSTLunorderedmap: - case ROOT::kSTLunorderedmultimap: - break; - default: - ValidateAddress(); // There is no ReadLeave for this node, so we need to do the validation here. - for (Int_t i = 0; i < nbranches; ++i) { - TBranch* branch = (TBranch*) fBranches.UncheckedAt(i); - Int_t nb = branch->GetEntry(entry, getall); - if (nb < 0) { - return nb; - } - nbytes += nb; + if (!IsAssociativeContainer(fSTLtype)) { + ValidateAddress(); // There is no ReadLeave for the node of a top level split object, so we need to do the + // validation here. + for (Int_t i = 0; i < nbranches; ++i) { + TBranch* branch = (TBranch*) fBranches.UncheckedAt(i); + Int_t nb = branch->GetEntry(entry, getall); + if (nb < 0) { + return nb; } - break; + nbytes += nb; + } } if (!TestBit(kDecomposedObj) && fReadActionSequence && !fReadActionSequence->fActions.empty()) { if (fType == 3) { @@ -2774,18 +2799,29 @@ Int_t TBranchElement::GetEntry(Long64_t entry, Int_t getall) } } else { // -- Terminal branch. + if (fBranchCount && (fBranchCount->GetReadEntry() != entry)) { Int_t nb = fBranchCount->TBranch::GetEntry(entry, getall); + if (nb < 0) + return nb; + nbytes += nb; + } + // For associative containers, the count branch is actually + // reading the subbranches (including this one) and in the case + // of a compiled proxy the necessary staging area is only available + // during the call to the count branch GetEntry. + // We detect this code path by checking the position of the cursor, + // which is explicitly reset by the count branch's GetEntry. + // One down-side, if the user call twice GetEntry on the same entry, + // the second call will not re-read the sub-branches (unlike all the other + // types of branches). + if (!fBranchCount || (prevEntry != entry) || !IsAssociativeContainer(fBranchCount->fSTLtype)) { + Int_t nb = TBranch::GetEntry(entry, getall); if (nb < 0) { return nb; } nbytes += nb; } - Int_t nb = TBranch::GetEntry(entry, getall); - if (nb < 0) { - return nb; - } - nbytes += nb; } if (R__unlikely(fTree->Debug() > 0)) { @@ -4064,23 +4100,14 @@ void TBranchElement::ReadLeavesMakeClass(TBuffer& b) } } fNdata = n[0]; - if (fType == 4) { + if (fType == 4 && IsAssociativeContainer(fSTLtype)) { Int_t nbranches = fBranches.GetEntriesFast(); - switch(fSTLtype) { - case ROOT::kSTLset: - case ROOT::kSTLmultiset: - case ROOT::kSTLmap: - case ROOT::kSTLmultimap: - for (Int_t i=0; iGetEntry(GetReadEntry(), 1); - if (nb < 0) { - break; - } - } - break; - default: + for (Int_t i=0; iGetEntry(GetReadEntry(), 1); + if (nb < 0) { break; + } } } return; @@ -4332,28 +4359,21 @@ void TBranchElement::ReadLeavesCollection(TBuffer& b) fIterators->CreateIterators(alternate, proxy); } - Int_t nbranches = fBranches.GetEntriesFast(); - switch (fSTLtype) { - case ROOT::kSTLset: - case ROOT::kSTLunorderedset: - case ROOT::kSTLunorderedmultiset: - case ROOT::kSTLmultiset: - case ROOT::kSTLmap: - case ROOT::kSTLmultimap: - case ROOT::kSTLunorderedmap: - case ROOT::kSTLunorderedmultimap: - for (Int_t i = 0; i < nbranches; ++i) { - TBranch *branch = (TBranch*) fBranches[i]; - Int_t nb = branch->GetEntry(GetReadEntry(), 1); - if (nb < 0) { - // Give up on i/o failure. - // FIXME: We need an error message here. - break; - } + if (IsAssociativeContainer(*proxy)) { + + Int_t nbranches = fBranches.GetEntriesFast(); + for (Int_t i = 0; i < nbranches; ++i) { + TBranch *branch = (TBranch*) fBranches[i]; + // To ensure that the sub-branches are read, we need + // reset their entry cursor. + RecursiveResetReadEntry(branch); + Int_t nb = branch->GetEntry(GetReadEntry(), 1); + if (nb < 0) { + // Give up on i/o failure. + // FIXME: We need an error message here. + break; } - break; - default: - break; + } } //------------------------------------------------------------------------ // We have split this stuff, so we need to create the pointers @@ -5134,7 +5154,7 @@ void TBranchElement::SetAddressImpl(void* addr, bool implied, Int_t offset) if(fSTLtype != ROOT::kSTLvector && fCollProxy->HasPointers() && fSplitLevel > TTree::kSplitCollectionOfPointers ) { fPtrIterators = new TVirtualCollectionPtrIterators(fCollProxy); - } else if (fCollProxy->GetProperties() & TVirtualCollectionProxy::kIsAssociative) { + } else if (IsAssociativeContainer(*fCollProxy)) { fWriteIterators = new TVirtualCollectionIterators(fCollProxy,false); fIterators = new TVirtualCollectionIterators(fCollProxy); } else { @@ -5176,7 +5196,7 @@ void TBranchElement::SetAddressImpl(void* addr, bool implied, Int_t offset) delete fPtrIterators; if(fSTLtype != ROOT::kSTLvector && fCollProxy->HasPointers() && fSplitLevel > TTree::kSplitCollectionOfPointers ) { fPtrIterators = new TVirtualCollectionPtrIterators(fCollProxy); - } else if (fCollProxy->GetProperties() & TVirtualCollectionProxy::kIsAssociative) { + } else if (IsAssociativeContainer(*fCollProxy)) { fWriteIterators = new TVirtualCollectionIterators(fCollProxy,false); fIterators = new TVirtualCollectionIterators(fCollProxy); } else { @@ -5212,7 +5232,7 @@ void TBranchElement::SetAddressImpl(void* addr, bool implied, Int_t offset) delete fPtrIterators; if(fSTLtype != ROOT::kSTLvector && fCollProxy->HasPointers() && fSplitLevel > TTree::kSplitCollectionOfPointers ) { fPtrIterators = new TVirtualCollectionPtrIterators(fCollProxy); - } else if (fCollProxy->GetProperties() & TVirtualCollectionProxy::kIsAssociative) { + } else if (IsAssociativeContainer(*fCollProxy)) { fWriteIterators = new TVirtualCollectionIterators(fCollProxy,false); fIterators = new TVirtualCollectionIterators(fCollProxy); } else { @@ -5282,7 +5302,7 @@ void TBranchElement::SetAddressImpl(void* addr, bool implied, Int_t offset) if (!fIterators && !fPtrIterators) { if(fSTLtype != ROOT::kSTLvector && GetCollectionProxy()->HasPointers() && fSplitLevel > TTree::kSplitCollectionOfPointers ) { fPtrIterators = new TVirtualCollectionPtrIterators(GetCollectionProxy()); - } else if (fCollProxy->GetProperties() & TVirtualCollectionProxy::kIsAssociative) { + } else if (IsAssociativeContainer(*fCollProxy)) { fWriteIterators = new TVirtualCollectionIterators(fCollProxy,false); fIterators = new TVirtualCollectionIterators(fCollProxy); } else { @@ -5855,7 +5875,7 @@ void TBranchElement::SetFillLeavesPtr() } else { fFillLeaves = (FillLeaves_t)&TBranchElement::FillLeavesCollectionSplitPtrMember; } - } else if (GetCollectionProxy()->GetProperties() & TVirtualCollectionProxy::kIsAssociative) { + } else if (IsAssociativeContainer(*GetCollectionProxy())) { fFillLeaves = (FillLeaves_t)&TBranchElement::FillLeavesAssociativeCollectionMember; } else { fFillLeaves = (FillLeaves_t)&TBranchElement::FillLeavesCollectionMember;