Skip to content

[cont] Clear "C" TClonesArray: Remove object from TProcessID-table ; Allow removing without destructing #18439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ferdymercury wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

[cont] Clear "C" TClonesArray: Remove object from TProcessID-table ; Allow removing without destructing #18439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ced4058
[cont] Clear "C" TClonesArray: Remove object from TProcessID-table
ferdymercury Apr 17, 2025
9ac9723
[cont] declare new clearslot function
ferdymercury Apr 17, 2025
b778609
[cont] try to fix test failures
ferdymercury Apr 17, 2025
118d9e2
[cont] Apriori we want the expansive part of the operation (Compress)…
ferdymercury Apr 17, 2025
f8fd22b
[test] implement tests for ClearSlot and RefArray clear
ferdymercury Apr 21, 2025
c8b5dd2
[nfc] clang-format and compiler error fixes
ferdymercury Apr 21, 2025
195634d
[nfc] fix more clangf and warnings
ferdymercury Apr 21, 2025
6373748
[nfc] clangformat and dependency fix
ferdymercury Apr 21, 2025
63c7edf
[test] move to tree folder
ferdymercury Apr 21, 2025
2d728f7
[test] fix dictionary generation
ferdymercury Apr 21, 2025
e6ba89c
[nfc] try fix dict generation
ferdymercury Apr 21, 2025
f712fe9
[nfc] clangf
ferdymercury Apr 21, 2025
b952a63
Apply suggestion from @ferdymercury
ferdymercury Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/cont/inc/TClonesArray.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class TClonesArray : public TObjArray {
TClonesArray& operator=(const TClonesArray& tc);
void Compress() override;
void Clear(Option_t *option="") override;
void ClearSlot(Int_t index, Option_t *opt = "");
void Delete(Option_t *option="") override;
void Expand(Int_t newSize) override;
virtual void ExpandCreate(Int_t n);
Expand Down
27 changes: 23 additions & 4 deletions core/cont/src/TClonesArray.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ When investigating misuse of TClonesArray, please make sure of the following:
#include "TClass.h"
#include "TObject.h"
#include "TObjectTable.h"
#include "TProcessID.h"
#include "snprintf.h"

#include <cstdlib>
Expand Down Expand Up @@ -430,9 +431,13 @@ void TClonesArray::Clear(Option_t *option)
TObject *obj = UncheckedAt(i);
if (obj) {
obj->Clear(cplus);
obj->ResetBit( kHasUUID );
obj->ResetBit( kIsReferenced );
obj->SetUniqueID( 0 );
if (obj->TestBit(TObject::kIsReferenced))
if (auto pid = TProcessID::GetProcessWithUID(obj)) {
pid->RecursiveRemove(obj);
obj->ResetBit(TObject::kIsReferenced);
}
obj->ResetBit(kHasUUID);
obj->SetUniqueID(0);
}
}
}
Expand All @@ -443,10 +448,24 @@ void TClonesArray::Clear(Option_t *option)
TObjArray::Clear();
}

////////////////////////////////////////////////////////////////////////////////
/// Clear a slot in the array with option opt, later nullify it and call Compress
/// \param index the index of the array to clear, it must be in range of length of fCont, range-check is not performed
/// \param opt the option passed to the Clear function
/// \note Consider calling Compress() after this operation
void TClonesArray::ClearSlot(Int_t index, Option_t *opt)
{
if (fCont[index]) {
fCont[index]->Clear(opt);
fCont[index] = nullptr;
Changed();
}
}

////////////////////////////////////////////////////////////////////////////////
/// Clear the clones array. Use this routine when your objects allocate
/// memory (e.g. objects inheriting from TNamed or containing TStrings
/// allocate memory). If not you better use Clear() since if is faster.
/// allocate memory). If not you better use Clear() since it is faster.

void TClonesArray::Delete(Option_t *)
{
Expand Down
2 changes: 2 additions & 0 deletions tree/tree/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ if(MSVC AND NOT CMAKE_GENERATOR MATCHES Ninja)
endif()
target_include_directories(testTOffsetGeneration PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
ROOT_STANDARD_LIBRARY_PACKAGE(SillyStruct NO_INSTALL_HEADERS HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/SillyStruct.h SOURCES SillyStruct.cxx LINKDEF SillyStructLinkDef.h DEPENDENCIES RIO)
ROOT_STANDARD_LIBRARY_PACKAGE(TestRefObj NO_INSTALL_HEADERS HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/TestRefObj.h SOURCES TestRefObj.cxx LINKDEF TestRefObjLinkDef.h DEPENDENCIES RIO)
ROOT_ADD_GTEST(testBulkApi BulkApi.cxx LIBRARIES RIO Tree TreePlayer)
#FIXME: tests are having timeout on 32bit CERN VM (in docker container everything is fine),
# to be reverted after investigation.
Expand All @@ -26,6 +27,7 @@ if(NOT CMAKE_SIZEOF_VOID_P EQUAL 4)
endif()
ROOT_ADD_GTEST(testTBasket TBasket.cxx LIBRARIES RIO Tree)
ROOT_ADD_GTEST(testTBranch TBranch.cxx LIBRARIES RIO Tree MathCore)
ROOT_ADD_GTEST(testClonesArray testClonesArray.cxx LIBRARIES RIO Tree TestRefObj)
ROOT_ADD_GTEST(testTIOFeatures TIOFeatures.cxx LIBRARIES RIO Tree)
ROOT_ADD_GTEST(testTTreeCluster TTreeClusterTest.cxx LIBRARIES RIO Tree MathCore)
ROOT_ADD_GTEST(testTChainParsing TChainParsing.cxx LIBRARIES RIO Tree)
Expand Down
4 changes: 4 additions & 0 deletions tree/tree/test/ElementStruct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#ifndef ELEMENTSTRUCT_H
#define ELEMENTSTRUCT_H
/**
* The ElementStruct has no purpose except to provide
* inputs to the test cases.
Expand All @@ -8,3 +10,5 @@ class ElementStruct {
int i;
double *d; //[i]
};

#endif
5 changes: 5 additions & 0 deletions tree/tree/test/SillyStruct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#ifndef SILLYSTRUCT_H
#define SILLYSTRUCT_H

#include "Rtypes.h"
#include "TObject.h"

Expand All @@ -15,3 +18,5 @@ class SillyStruct : public TObject {

ClassDefOverride(SillyStruct, 1)
};

#endif
1 change: 1 addition & 0 deletions tree/tree/test/TestRefObj.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include "TestRefObj.h"
22 changes: 22 additions & 0 deletions tree/tree/test/TestRefObj.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#ifndef TESTREFOBJ_H
#define TESTREFOBJ_H

#include "TRef.h"
#include "TRefArray.h"

// https://its.cern.ch/jira/browse/ROOT-7249
class TestRefObj : public TObject {
protected:
TRefArray lChildren;

public:
TestRefObj() : lChildren() {}
virtual ~TestRefObj() {}
virtual void Clear(Option_t *opt = "C") { lChildren.Clear(opt); }

Check failure on line 15 in tree/tree/test/TestRefObj.h

View workflow job for this annotation

GitHub Actions / debian13 dev=ON, CMAKE_CXX_FLAGS=-Wsuggest-override 

‘virtual void TestRefObj::Clear(Option_t*)’ can be marked override [-Werror=suggest-override]

Check failure on line 15 in tree/tree/test/TestRefObj.h

View workflow job for this annotation

GitHub Actions / debian125 CMAKE_CXX_STANDARD=20, dev=ON, CMAKE_CXX_FLAGS=-Wsuggest-override 

‘virtual void TestRefObj::Clear(Option_t*)’ can be marked override [-Werror=suggest-override]
virtual void SetChild(TestRefObj *aChild) { lChildren.Add(aChild); }
virtual const TestRefObj *GetChild(Int_t idx = 0) const { return static_cast<TestRefObj *>(lChildren.At(idx)); }
virtual Bool_t HasChild() const { return (lChildren.GetEntriesFast() > 0); }
ClassDef(TestRefObj, 1);
};

#endif
9 changes: 9 additions & 0 deletions tree/tree/test/TestRefObjLinkDef.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#ifdef __CLING__

#pragma link off all globals;
#pragma link off all classes;
#pragma link off all functions;

#pragma link C++ class TestRefObj+;

#endif
120 changes: 120 additions & 0 deletions tree/tree/test/testClonesArray.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
#include "TObject.h"
#include "TestRefObj.h"
#include "TFile.h"
#include "TTree.h"
#include "TRandom.h"
#include "TClonesArray.h"
#include "TProcessID.h"
#include "TSystem.h"

#include "gtest/gtest.h"

// https://its.cern.ch/jira/browse/ROOT-7249
TEST(TClonesArray, RefArrayClearChildren)
{
Bool_t resetObjectCount = kTRUE;
Bool_t activateBranchRef = kFALSE;
Int_t splitLevel = 1;
Bool_t pruneSecondChildren = kTRUE;
auto filename = "test7249.root";
auto treename = "tree";
// streamwithrefs
{
TFile testFile(filename, "RECREATE");
TTree dataTree(treename, treename);

TClonesArray particles(TestRefObj::Class(), 100);
TClonesArray children(TestRefObj::Class(), 100);

dataTree.Branch("particles", &particles, 32768, splitLevel);
dataTree.Branch("children", &children, 32768, splitLevel);
if (activateBranchRef) {
dataTree.BranchRef();
}

for (Int_t e = 0; e < 1000; e++) {
// For each "event".
UInt_t objCount = TProcessID::GetObjectCount();

TestRefObj *motherPart = static_cast<TestRefObj *>(particles.ConstructedAt(0));
TestRefObj *childPart = static_cast<TestRefObj *>(children.ConstructedAt(0));
motherPart->SetChild(childPart);

// Prune all children if requested and event odd.
if (pruneSecondChildren && (e % 2 != 0)) {
children.Clear("C");
}

Copy link
Collaborator Author

@ferdymercury ferdymercury Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olifre could you explain at this line how your "second workaround" was working? I guess that, before calling dataTree.Fill(), you were walking in a for-loop the motherPart.GetChild() checking whether the stored process ID still existed in the static TProcess table ?

Copy link
Contributor

@olifre olifre Apr 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferdymercury Actually, since this was in an experiment-specific framework and I did not trust all users to do it right, I chose the following approach:

  • Have two classes inheriting from TRef and TRefArray.
  • Via an interface class (to prevent code duplication), these two classes had a hook when any reference was set, injecting themselves into a global static list of instances referencing objects.
  • Before writeout, this static list of referencing instances is walked and each instance is asked to check the references, i.e. in the case of the class inheriting form TRefArray:
void CBTRefArray::fMaybePruned() {
        for (Int_t i = 0; i < fSize; i++) {
                if (fUIDs[i]) {
                        /* Re-check reference-target, might have become NULL due to pruning -
                         * in that case, store that in our UniqueID.
                         */
                        if (At(i) == nullptr) {
                                fUIDs[i] = 0;
                        }
                }
        }
}

(and the same without loop for TRef).

So in the end, it was the loop you described, but actually the TRef classes themselves were checking their reference targets and in case these had become nullptr they adapted their internally stored UID before writeout.
I'm not sure this can realistically be put into the Streamers, as the reference target may have been written out and deleted from memory already, which is why I used a "user hook before writeout" to trigger this check.

Copy link
Collaborator Author

@ferdymercury ferdymercury Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

A bit related issue: https://its.cern.ch/jira/browse/ROOT-6077

dataTree.Fill();

children.Clear("C");
particles.Clear("C");

if (resetObjectCount) {
TProcessID::SetObjectCount(objCount);
}
}
dataTree.Write();
testFile.Close();
}
// readwithrefs
{
TFile testFile(filename, "READ");
TTree *dataTree = testFile.Get<TTree>(treename);

TClonesArray *particles = nullptr;
TClonesArray *children = nullptr;

dataTree->SetBranchAddress("particles", &particles);
dataTree->SetBranchAddress("children", &children);

if (activateBranchRef) {
dataTree->BranchRef();
}
const Long64_t entries = dataTree->GetEntriesFast();
for (Long64_t e = 0; e < entries; e++) {
dataTree->GetEntry(e);

// For each "event".
UInt_t objCount = TProcessID::GetObjectCount();

// UInt_t parts = particles->GetEntries();
UInt_t childs = children->GetEntries();

auto parti = static_cast<TestRefObj *>(particles->UncheckedAt(0));
if (pruneSecondChildren && (e % 2 != 0)) {
ASSERT_EQ(childs, 0);
ASSERT_FALSE(parti->HasChild());
} else {
ASSERT_NE(childs, 0);
ASSERT_TRUE(parti->HasChild());
}

children->Clear("C");
particles->Clear("C");

if (resetObjectCount) {
TProcessID::SetObjectCount(objCount);
}
}

delete dataTree;
testFile.Close();
}
gSystem->Unlink(filename);
}

// https://its.cern.ch/jira/browse/ROOT-7473
TEST(TClonesArray, ClearSlot)
{
TClonesArray particles(TestRefObj::Class(), 100);
TClonesArray children(TestRefObj::Class(), 100);
TestRefObj *motherPart = static_cast<TestRefObj *>(particles.ConstructedAt(0));
TestRefObj *childPart = static_cast<TestRefObj *>(children.ConstructedAt(0));
motherPart->SetChild(childPart);
particles.ClearSlot(0, "C");
auto parti = static_cast<TestRefObj *>(particles.UncheckedAt(0));
ASSERT_EQ(parti, nullptr);
auto child = static_cast<TestRefObj *>(children.UncheckedAt(0));
ASSERT_NE(child, nullptr);
}
Loading