Faster default/copy construction of TObjects #482
Conversation
|
Can one of the admins verify this patch? |
|
complimentary info: For this test program #include "TObject.h"
class Track : public TObject {
public:
Track() : TObject() {}
virtual ~Track() = default;
private:
double x=0.;
double y=0.;
double z=0.;
};
// noinline for valgrind purposes
__attribute__((noinline)) void createtracks(std::vector<Track> &v, int N) {
for(int i=0;i<N;++i){
v.emplace_back();
}
}
int main() {
// switch off object stat
TObject::SetObjectStat(false);
int N=1000000;
std::vector<Track> tracks;
tracks.reserve(N);
createtracks(tracks, N);
return 0;
}the patch decreases the CPU cycle count for the function |
|
@Axel-Naumann : see complimentary info. The compiler voluntarily inlines this. |
|
@phsft-bot build! |
|
Starting build on |
core/base/inc/TObject.h
Outdated
|
|
||
| fUniqueID = 0; | ||
|
|
||
| if (fgObjectStat) TObject::AddToTObjectTable(this); |
There was a problem hiding this comment.
For an additional performance boost, try,
if (R__unlikely(fgObjectStat)) TObject::AddToTObjectTable(this);
There was a problem hiding this comment.
will do.... thanks for the suggestion
core/base/src/TObject.cxx
Outdated
| @@ -158,6 +139,11 @@ TObject::~TObject() | |||
| if (fgObjectStat && gObjectTable) gObjectTable->RemoveQuietly(this); | |||
| } | |||
|
|
|||
| void TObject::AddToTObjectTable(TObject *op) | |||
There was a problem hiding this comment.
Could you add a documentation/comment for this routine (et/or a raison d'etre)?
There was a problem hiding this comment.
Can do. It is here because I could not use directly TObjectTable::AddObj because this would introduce a cyclic dependency of headers.
There was a problem hiding this comment.
I think this is the last missing piece (doc for functions is required per coding convention).
|
@vgvassilev "convert the example program into a unit test?" |
|
@sawenzel This is very nice thanks. For consistency's sake, shouldn't we also move the copy constructor and operator=? Thanks. |
|
Ok, I see, I got the impression this is more than moving a method inline. @sawenzel please ignore my request. |
sawenzel
force-pushed
the
swenzel/TObjectconstr
branch
from
a63ad3e to
2d2b565
Compare
|
@pcanal : Thanks for these suggestions. I made a new version. |
sawenzel
changed the title
core/base/inc/TObject.h
Outdated
|
|
||
| inline TObject &TObject::operator=(const TObject &rhs) | ||
| { | ||
| if (this != &rhs) { |
There was a problem hiding this comment.
probably would also benefit from
if (R__likely(this != &rhs)) {
Cheers, Philippe.
sawenzel
force-pushed
the
swenzel/TObjectconstr
branch
from
2d2b565 to
d6ac940
Compare
|
@sawenzel Could you also add a (summarized) version of the 'complementary info' in the commit log and/or code-comments? |
sawenzel
force-pushed
the
swenzel/TObjectconstr
branch
from
d6ac940 to
1810e10
Compare
|
@pcanal : done |
sawenzel
force-pushed
the
swenzel/TObjectconstr
branch
3 times, most recently
from
61446d7 to
da003dd
Compare
* This commit allows the compiler to potentially inline/optimize construction of TObjects * This is in particular important for data-objects which are created billions of times and which inherit from TObject The effects of these changes were shown to be visible in a test study. Creation of a large amount of simple objects deriving from TObject benefited from a ~30% speed improvement (as measured by valgrind callgrind).
|
This PR breaks roottest-root-io-filemerger-make. The symptom is a different size of the TStreamerInfo record on disk. I am reverting the commit in order to stabilise the CI of ROOT. We need to further investigate why this apparently unrelated change has an effect given the very delicate area where the problem has been identified. |
|
Thanks for this information. Is there a timeline to investigate the issue? |
|
Hi, we plan to branch for 6.10 on May the 10th. |
|
Thanks Danilo. |
inline/optimize construction of TObjects
which are created billions of times and which inherit from
TObject