[WIP] A new type-safe and casting-free wrapper for TClonesArray: TCAViewer

[YanzhaoW]

Motivation

Much has been discussed about castings during event loops. Ideally casting should not be used at all. The biggest reason why there are so many castings in our program is because all the input and output data are in the type TClonesArray, which neither has type-safety nor memory safety. Some people, like our former colleague, @janmayer, tried to create type-safety by making a wrapping class template, called TCAConnector. But unfortunately, in the actual implementation, data pointers still need to be copied and casted to the correct version.

The new wrapper TCAViewer solves this problem by using an array-view class called span. The idea of viewing an array is relatively new and was not even implemented until the latest standard C++20. With C++17, we can still fully utilize such a conception from a library called "Guideline Supported Library" (GSL).

Basic idea

Unlike std::array<>, gsl::span (or std::span in C++20) is very light weighted and does not own any data. It just requires the address of the first element and the size of the container. However, it has all features of modern C++ containers, such as iterators and range-based looping. Fortunately, the address of the first element of TClonesArray always stays the same throughout all events. Once such address value becomes known, a span object can be created per event to represent all the data from TClonesArray without the need to copy or cast.

TCAViewer

(please check the source code in TCAViewer.h for the full details)

Initialisation:

• Input mode:

auto inputData = TCAViewer::Data<R3BNeulandPoint, TCAViewer::in>("NeulandPoints");

• Output mode:

auto outputData = TCAViewer::Data<R3BNeulandHit, TCAViewer::out>("NeulandHits");

Return:

• Input mode:
  Note: Always keep in mind that the return in input mode is a span containing pointers to the data, NOT directly data.

auto inputPtrs = inputData.Get(); // inputPtrs has the type gsl::span<R3BNeulandPoint*>

//iterator:
for(auto itr = inputPtrs.begin(); itr != inputPtrs.end(), itr++)
{
    (*itr)->GetLightYield();
    //...
}

// range-based looping
for(const auto& it : inputPtrs)
{
   it->GetLightYield();
}

• Output mode:

auto outputPtr = outputData.Get(); // outputPtr has the type TCAViewer::TCAOutput_SafeGuard<R3BNeulandHit>

outputPtr.Emplace_back( R3BNeulandHit() );
outputPtr.Emplace_back( R3BNeulandHit() );
outputPtr.Emplace_back( R3BNeulandHit() );
//...

Data types:

There are three important data types defined in TCAViewer:

• TCAViewer::Mode
• TCAViewer::Data
• TCAViewer::TCAOutput_SafeGuard

TCAViewer::Mode is an enumerator containing only two entries: TCAViewer::in and TCAViewer::out. It specifies whether the object from the class TCAViewer::Data represents input data or output data. If you accidentally choose the wrong mode, an error will be shown during the compilation (see below).

TCAViewer::Data, as a class template, is the main part of TCAViewer. Its signature and constructor are shown below:

template <typename DataType, Mode mode = in>
class Data
{
  public:
    explicit Data(std::string branchName = "");
}

The typename DataType is the type of the object, such as R3BNeulandPoint or TVector3, which must be derived from TObject. mode shows whether you need an input data or an output data. The constructor parameter branchName is the name of the branch where the actual data is extracted from TTree. It's the same constructor parameter when you use type-unsafe FairRootManager::GetObject(), such as

fMappedTrigger = (TClonesArray*)mgr->GetObject("NeulandTrigMappedData");

TCAViewer::TCAOutput_SafeGuard is a return type of TCAViewer::Pointer<DataType, TCAViewer::out>::Get() and should rarely appear if you have a good C++ programming habit (using auto as much as possible. ). Remember always needing to "clear" the TClonesArray or TCAOutputConnector at the end of the event loop? With TCAViewer, there is no such need as this is automatically done (thanks to an C++ idiom called "RAII").

Member functions of TCAViewer::Data:

To be added...

UB?

if (dynamic_cast<DataType*>((*mTCA)[0]) == nullptr)
 {
     LOG(fatal) << "cannot convert the element in " << mBranchName << " to the type "
           << DataType::Class_Name();
}
else
{
    mFirstAddr = reinterpret_cast<DataType**>(&(*mTCA)[0]);
    is_success = true;
}

To be continued...

[klenze]

Some comments:

• boost 1.78 (which is included in the previous-to-latest FairSoft apr22, which is soon going to be the minimally required version of FS for R3BRoot) seems to contain a std::span complatible std::span. I would prefer if you used that. Introducing new dependencies -- even if they are very innocent small libraries -- brings us closer to dependency hell.
• TClonesArray::fCont will not be constant over the lifetime of a TCA. It is, after all, a C array which can only hold so many events. If it becomes to small, TObjArray::Expand gets called, which will use (a ROOT-specific version of) realloc, which can assign a new memory location to it. I think that the correct solution is to fetch tca[0] for every call.
• Related: OutputGet()->Emplace() can invalidate any spans from InputGet() if realloc changes the memory location. Thus people can not safely use OutputGet and InputGet objects at the same time, which is inconvenient. (If you really want, you could have an integer nReaders and assert that fCont is unchanged for nReaders>0, which means people who want both in and out at the same time which would then have to explicitly call TCA::Expand to set a suitable size before calling OutputGet.)
• Taking the address of a reference is breaking the implementation/interface boundary as much as using #define private public and accessing TClonesArray::fCont directly would. I am not a purist who is telling people to never break abstraction, sometimes an ugly problem calls for an ugly fix, but it certainly comes at a cost. e.g. if ROOT got rid of the reference in TObject *&TClonesArray::operator[](Int_t idx), your code would still compile but your mFirstAddr would point to a stack location.
• Your reinterpret_cast depends on sizeof(DataType*)==sizeof(TObject*), which seems reasonable enough. (I was also thinking about multiple inheritance cases where size_t(static_cast<TObject*>(myDataPtr))!=size_t(myDataPtr), but I became quite sure that any object which inherits from another class before inheriting from TObject derived one is unsafe to construct in a TCA in any case.)
• At the moment it looks like a user could change the contents of fCont by using myspan[3]=nullptr;. You would probably want to use reinterpret_cast<DataType* const *> (K&R clearly did not envision qualifiers in their language, or they would have found ptr<ptr<int>> much preferable to int**) and gsl::span<DataType* const>.
• As anything you could do with an Input viewer can also be done with an Output viewer, one might want to use inheritance there.
• Your clearing of the TCA only happens during the next event. If someone would write

 if (nHits>0)
 {
       auto outputPtr = outputData.Get();
       ...
  }

then it would never happen for events with nHits==0. (FairTask does not provide a hook which is run at the start of an event, which would be the perfect time to reset stuff.)

Otherwise, this is the third implementation of a wrapper for TCAs in our codebase:

• Jans converts the contents to a std::vector<DataType*>
• mine uses wrapped TIterators (uses trickery to get around the need for an explicit call during Init())
• Yours breaks encapsulation to directly access fCont (probably fastest)

All of these have upsides and downsides.

Has anyone recently tried to see of ROOT can nowadays store std::vector<TObjectDerivedType> directly in a ROOT file? If yes, it would be preferable to do things right from the start.

[YanzhaoW]

@klenze

Thanks for your feedback. These are very helpful.

• Sure, if we could use std::span in boost, I'm glad to change to that. [FIXED]
• Yeah, I also had this concern. What I could do is to check whether the value from TClonesArray::Capacity() is changed or not. If it's changed, I then check the fCont value. Do you think this is plausible? [FIXED]
• I'm not sure I understand this. InputGet() and OutputGet() are two functions from two different classes (they do come from same class template.) If you have one object with input mode, it cannot call OutputGet(). Same for the object with output mode and InputGet(). Or do you mean one input data share the same branch name with the output data? If so, I'm not sure whether this is possible with native TClonesArray. But I can create another Mode called inout for those data which people want to edit, instead of only reading or writing.
• Maybe I should use GetObjectRef(), which directly gives me fCont? [FIXED]
• size of a pointer, I guess, is generally fixed in 64 or 32 bit machine. It could be changed in older machine.But for the safety, I can still check once for all whether the size of pointer pointing to DataType is equal to the size of pointer pointing to TObject. [FIXED]
• Yeah, for reading the DataType definitely should be const. [FIXED]
• I wouldn't say so. TCAViewer::Data<DataType, TCAViewer::in> is a different class from TCAViewer::Data<DataType, TCAViewer::out>. The return types from these two modes are also very different.
• Yeah, that's a hard issue. I will think about what I can do to discourage such behaviour in the code. But from the first glance, it looks extremely difficult.[FIXED]

Has anyone recently tried to see of ROOT can nowadays store std::vector directly in a ROOT file? If yes, it would be preferable to do things right from the start.

For this, we should wait for ROOT 7.

[YanzhaoW]

@klenze

For the last point, no, there is no simple and clean solution I could come up with now. The ugly and easy way I could think of is to check the event number. Check whether the difference between the event number when the Get() is called last time and this time is 1 or not. If not, raise an error.

But such risk should be pointed out during the compile time not the run time. I don't know how to check this in compile time.

[janmayer]

emplace_back if you want to mimic std, or EmplaceBack if you want to keep the style of the other names in this class.

[YanzhaoW]

Ok, I will follow emplace_back.

[janmayer]

Sure you want to provide a default here? If there is no usecase for it, I would also not provide getter and setter for mBranchName.

[YanzhaoW]

Yeah, better write a default one. Setter is kind of needed especially when branch name is not available during inline initialisation.

[janmayer]

I'm not sure that it is wise to have both input and output in the same class, as there is some complexity involved with if-fing between those modes. There seem to be two distinct classes in one here. One of which does not need the SafeGuard (if I saw that correctly).

[YanzhaoW]

Yeah, they are different classes with different methods. But I would say for users, they are just two kinds of data handling. One could be input or output. So in terms of usability, IMO, TCAVIewer<out> and TCAViwer<in> is better than TCAOutputViewer and TCAInputViewer.

Another reason is I'm still considering how to add another mode call "inout". With this mode, user can modify the data (read and write), which requires both functionalities from in mode and out mode.

[janmayer]

I think I get what this is for, but you do need a small comment on why it is needed.

[YanzhaoW]

ok, yeah, I should do that.

[janmayer]

I think LOG(fatal) aborts, but you might want to make this more explicit.

Do what the editor tells you to do: If you add a exception below the log, and the code analysis says it can't be reached, then you don't need it (or just a comment).

[YanzhaoW]

You are right. Error handling is my weakest spot in C++. Maybe I should throw an exception instead of using "LOG(fatal)"?

[janmayer]

As the log(fatal) acts as an "early exit", you don't need the else, and can put inner scope on one level with the return.

[YanzhaoW]

Yes, make sense

[janmayer]

don't need that function, its just a wrapper

[YanzhaoW]

ok

[janmayer]

Log... is not what this does. This is about ensuring the function was called, right? So what about EnsureFunctionCalled or similar?
Then, you could pull it together with the safeguard, and pull out the if safeguard plus theEnsureFunctionCalled from OutputGet into a EnsureFunctionCalledExactlyOnce.

I don't understand why it is necessary, by the way. :-)

[janmayer]

[[nodiscard]] auto OutputGet() -> TCAOutput_SafeGuard<DataType>
{
    EnsureFunctionCalledExactlyOnce();
    Clear();
    return TCAOutput_SafeGuard<DataType>(this);
}

[YanzhaoW]

Yes. The reason is when user put this Get() (or OutputGet) function call inside an if-statement or there is a throw before the function call, Get() function won't be called for every event. And in some events, if function is not called, old data from last call can't be cleared and as the result they would be written down to files for the second time in the end of event loop. This is bad. (I also think clearing data cache in TClonesArray should be handled by FairRootManger, not by ourself.)

So I need to make sure the OutputGet() function must be called at least once to clear the old data. And I also don't want user to create multiple unnecessary output handlers for a single output data. The function should be called exactly once per event loop. Yeah, EnsureCalledOnce is a better name.

[jose-luis-rs]

Please, rebase the dev branch to update the CIs

[YanzhaoW]

Please, rebase the dev branch to update the CIs

There are still some changes to I need to make. I will inform you when it's finished.

[klenze]

NB: In #792, I have moved the core tasks for CALIFA (except CalifaPoint for simulation) to std::map and std::vector. So far, this seems to work. I think the preferable way to use TClonesArrays is "not at all".

[YanzhaoW]

NB: In #792, I have moved the core tasks for CALIFA (except CalifaPoint for simulation) to std::map and std::vector. So far, this seems to work. I think the preferable way to use TClonesArrays is "not at all".

@klenze Yes, I also managed to store a std::vector using FairRootManager::RegisterAny. It works pretty good. But why the hell does RegisterAny take a reference of a pointer?! Basically I can't directly give the address of the data like:

class MyClass : FairTask
{
      std::vector<NeulandMappedData> data_;
}

FairRootManager::Instance()->RegisterAny("a name", &data_, true);

But rather I have to do something ugly like this:

class MyClass : FairTask
{
      std::vector<NeulandMappedData> data_;
      std::vector<NeulandMappedData>* dataPtr_;

     // constructor:
    {
           dataPtr_ = &data_;
    }
}

FairRootManager::Instance()->RegisterAny("a name", dataPtr_, true);

It's so hard to have really nice things with FairRoot :(

[klenze]

RegisterAny actually applies the & operator to its argument:

void FairRootManager::RegisterAny(const char* brname, T*& obj, bool persistence)
{
   ...
   AddMemoryBranchAny<T>(brname, &obj);
   ...
}

Kids, don't do this at home.

InitObjectAs feels confusing as well, the comment says "Initializes and returns a default object for a branch", but from reading the code I would assume it actually first searches in its branches, then asks fSource if it happens to know an object with that name, and if that fails, it actually returns nullptr, which is what I would want?

Have you tried using anything except for std::vector as a container? I tried std::map and it did not work to well for me. ROOT seems to serialize std::map<K,V> to an array of tuple<K,V> or something, and I did not succeed in getting it to turn it back into a map when reading. The documentation for serializing STL to *.root felt rather slim. Of course, it was probably a year ago that I tried it, might be better now.

If std::map does not work, I guess one could write a custom class and just reconstruct the map during deserialization.

[YanzhaoW]

@klenze Yes, I just tried writing and reading from std::map. It works pretty much the same as std::vector. Did you add the definition to the cint link file? Like:

#pragma link C++ class map<int, R3BPaddleTamexMappedData2>+;

And probably adding #include <map> on the top could also help.

RegisterAny actually applies the & operator to its argument

Yes, thus if I just pass &data, it's referring to a temporary object, which will be deleted in the next line. I see the reason for this atrocity. So when we want to read a vector from a root file using SetBranchAddress, we need to give a pointer to the resource, which is stored as a pointer to the vector:

auto data = std::vector<MapData>{};
auto* ptr = &data;
tree->SetBranchAddress("name", &ptr);

But anyway, we could throw the TCloneArray into trash bin from now.

[klenze]

Yes, I just tried writing and reading from std::map

Interesting. did you read back the map from file with FairRoot or with plain ROOT? Do you know if ROOT performs type checking? (If it is based on TTree::SetAddress(void*), I don't see how it would do that).
I kind of wish that the interface for fetching existing branches of trees was like the templated TDirectory::Get. And all this "pass the address of a pointer which points to a an STL structure in TTree::Branch" is probably why FairRoot does it like that. I don't get that at all. Either the user creates the STL container, then a reference to the container object should be plenty enough for TTree. Or the TTree allocates the STL object, then just take the desired type as an explicit template argument and return the object by reference.

The manual states

In case of dynamic structures changing with each entry for example, one must redefine the branch address before filling the branch again. This is done via the TBranch::SetAddress member function.

This is confusing to me. Does that mean that for a std::list with five elements, SetAddress actually remembers five different heap locations, and if I later have four or six elements in that list, I explicitly need to update them? If so, this is totally not how I would use containers (e.g. query the STL object for its contents in each TTree::Fill).

[YanzhaoW]

I close this PR because there is a better way to replace TClonesArray with STL containers. Please see #890.