Skip to content
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.

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

Support for emplace_back in Collections #271

Open
wdconinc opened this issue Mar 24, 2022 · 2 comments
Open

Support for emplace_back in Collections #271

wdconinc opened this issue Mar 24, 2022 · 2 comments

Comments

@wdconinc
Copy link
Contributor

Although a compiler is likely smart enough to optimize this, we would like to avoid the anti-pattern of using push_back to copy a temporary object into a collection, i.e.

        eicd::TrackerHit hit{ahit.getCellID(), // Raw DD4hep cell ID
                             {static_cast<float>(pos.x() / mm), static_cast<float>(pos.y() / mm),
                              static_cast<float>(pos.z() / mm)},                    // mm
                             {get_variance(dim[0] / mm), get_variance(dim[1] / mm), // variance (see note above)
                              std::size(dim) > 2 ? get_variance(dim[2] / mm) : 0.},
                              static_cast<float>(ahit.getTimeStamp() / 1000), // ns
                              m_timeResolution,                            // in ns
                              static_cast<float>(ahit.getCharge() / 1.0e6),   // Collected energy (GeV)
                              0.0F};                                       // Error on the energy
        rec_hits->push_back(hit);

by using emplace_back as

        rec_hits->emplace_back(ahit.getCellID(), // Raw DD4hep cell ID
                               {static_cast<float>(pos.x() / mm), static_cast<float>(pos.y() / mm),
                                static_cast<float>(pos.z() / mm)},                    // mm
                               {get_variance(dim[0] / mm), get_variance(dim[1] / mm), // variance (see note above)
                                std::size(dim) > 2 ? get_variance(dim[2] / mm) : 0.},
                                static_cast<float>(ahit.getTimeStamp() / 1000), // ns
                                m_timeResolution,                            // in ns
                                static_cast<float>(ahit.getCharge() / 1.0e6),   // Collected energy (GeV)
                                0.0F);                                       // Error on the energy

I don't know how easy it is to implement this considering relations etc. Even though an emplace_back(Args&&... args) doesn't ostentatiously need to know about the structure of the type, that may not be true at the lower levels, considering the implementation of push_back is not a simple pass through either...

@tmadlener
Copy link
Collaborator

Just from a technical point of view the copy of the temporary object is really just copying a pointer and increasing a reference count. (Explained in a bit more detail in this comment).

What could work could be an implementation in terms of the variadic create function, as that should have everything that is necessary. However, I want to point out that even in that case the underlying object will still be created on the heap and the collection just stores the pointer to it internally. In that sense it is essentially equivalent in terms of work as the temporary + push_back minus a few reference count manipulations.

If I understand correctly, you would mainly want this as a way to avoid anti patterns in general c++ code, and not primarily for performance reasons.

@wdconinc
Copy link
Contributor Author

Thanks for the additional detail. I have to admit that I am still mostly using podio, not necessarily always aware of the underlying structure. Based on what you say, it is indeed unlikely to result in a performance gain, but it would be useful to avoid the anti-pattern in gaudi algorithms where we want developers to use emplace_back for other containers..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants