[core] Pass TString by value to TNamed ctor #15469

ChristianTackeGSI · 2024-05-09T12:17:44Z

This Pull request:

Passing by const-reference can mean two constructions (once on the caller, once in the ctor using the TString copy ctor).

Instead passing by value and using move semantics in the ctor can reduce this to one construction on the caller and one move in the ctor.

Checklist:

tested changes locally
updated the docs (if necessary)

fixes: #15434

core/base/inc/TNamed.h

github-actions · 2024-05-09T14:36:41Z

Test Results

11 files 11 suites 2d 16h 17m 3s ⏱️
2 645 tests 2 645 ✅ 0 💤 0 ❌
27 455 runs 27 455 ✅ 0 💤 0 ❌

Results for commit 2bafc6e.

♻️ This comment has been updated with latest results.

pcanal · 2024-05-09T19:38:22Z

core/base/inc/TNamed.h

 TNamed(const char *name, const char *title) : fName(name), fTitle(title) { }
- TNamed(const TString &name, const TString &title) : fName(name), fTitle(title) { }
+ TNamed(TString name, TString title) : fName(std::move(name)), fTitle(std::move(title)) {}


Wouldn't

Suggested change

TNamed(TString name, TString title) : fName(std::move(name)), fTitle(std::move(title)) {}

TNamed(const &TString name, const &TString title) : fName(name), fTitle(title) {}

TNamed(TString &&name, const TString &title) : fName(std::move(name)), fTitle(title) {}

TNamed(const TString &name, TString &&title) : fName(name), fTitle(std::move(title)) {}

TNamed(TString &&name, TString &&title) : fName(std::move(name)), fTitle(std::move(title)) {}

be even more performant? (Albeit annoying :) )

Please take a look at the discussion in #15434. Especially https://www.cppstories.com/2018/08/init-string-member/, which analyzes nearly every combination.

A const-reference is not any more performant then a plain TString (assuming move is very cheap).

Has this been tested? Either by looking at the compiler output of some "real" code or by benchmarking?
Whenever performance and compiler optimizations are involved it's always best to verify the theoretical claims since it's very hard to guess correctly.

Especially cppstories.com/2018/08/init-string-member, which analyzes nearly every combination.

Actually they seems have left the case of the rvalue reference/universal forwarding as an exercise to the reader ("For now, I’d like to stop here and don’t go into details. ... " ) ;)

Whenever performance and compiler optimizations are involved it's always best to verify the theoretical claims since it's very hard to guess correctly.

Agreed. Also note that TString does have a "Small String Optimization (SSO)" which may or may not change things.

ChristianTackeGSI · 2024-06-04T15:40:14Z

@guitargeek should I rebase or something?

guitargeek · 2024-06-04T15:49:12Z

I was just closing and opening to refresh the test results. I'm baffled that there are failures in what seems like a trivial PR!

guitargeek · 2024-06-04T15:50:19Z

Maybe we uncovered a problem with the TString move constructor? Any intuition, @pcanal?

ChristianTackeGSI · 2024-06-04T16:12:52Z

Rebased, so that the tests run against the latest version. Maybe it''s getting better?

pcanal · 2024-06-04T16:14:06Z

Maybe we uncovered a problem with the TString move constructor?

It's implementation is odd looking :(, so yeah I think this is the issue ... let me do a quick checks

pcanal · 2024-06-04T16:28:52Z

It's implementation is odd looking :(, so yeah I think this is the issue ... let me do a quick checks

and it actually seems to be working ... so now I'll try to reproduce the failure.

pcanal · 2024-06-04T22:37:21Z

I can reproduce the failure locally.

pcanal · 2024-06-05T00:04:13Z

core/base/inc/TNamed.h

@@ -33,9 +33,9 @@ class TNamed : public TObject {
 TString fTitle; //object title

 public:
- TNamed(): fName(), fTitle() { }
+ TNamed() = default;


Suggested change

TNamed() = default;

// The following is *not* the default implementation (i.e. do not use `= default`.

// Default implementation can do 'more' than expected. In this case it reset all the

// data member of `TNamed` and more importantly its base class `TObject`.

// TObject's implementation of the `IsOnHeap` bit requires the memory occupied

// by `TObject::fUniqueID` to *not* be reset between the execution of `TObject::operator new`

// and the `TObject` constructor (Finding the magic pattern there is how we can determine

// that the object was allocated on the heap.

TNamed() : fName(), fTitle() { }

pcanal

Please apply https://github.com/root-project/root/pull/15469/files#r1626763205

ChristianTackeGSI · 2024-06-05T09:39:00Z

I can add those comments to my PR. I am wondering, whether TNamed is the only affected class?

If not, then this warning should be put in some central place instead of scattered around here?

If I get this correctly, it could also affect user classes? I have opened a topic on the forum about this to discuss this outside this PR.

pcanal · 2024-06-05T14:12:57Z

I can add those comments to my PR. I am wondering, whether TNamed is the only affected class?

That is a good question.

If not, then this warning should be put in some central place instead of scattered around here?

My guess it that it would likely affect at least all classes that derives directly from TObject.

pcanal · 2024-06-05T15:03:54Z

So this problem has been known for 5 years :( #4320 but we manage to indeed lose track of it. That PR used the following more concise pattern:

TView() {} // NOLINT: not allowed to use = default because of TObject::kIsOnHeap detection, see ROOT-10300

where both the NOLINT is indeed useful to avoid spurious tool recommendation and the wording was clear but should probably be updated (ROOT-10300 is a ticket number in the JIRA instance which is now read-only)

It would be good to add some wording in the TObject documentation.

ChristianTackeGSI · 2024-06-05T17:58:34Z

So please suggest a concise wording for this PR and I will change it. I'd probably go for

  TNamed() {}   // your-wording

then.

core/base/inc/TNamed.h

pcanal · 2024-06-05T19:55:49Z

For the TObject documentation I would add something along the line of:

Classes derived from `TObject` can not use the `= default` syntax for their constructor as some compilers implement optimizations that prevents the `TObject::kIsOnHeap` detection mechanism from working properly.

ChristianTackeGSI · 2024-06-06T08:27:27Z

For the TObject documentation I would add something along the line of:

Classes derived from `TObject` can not use the `= default` syntax for their constructor as some compilers implement optimizations that prevents the `TObject::kIsOnHeap` detection mechanism from working properly.

Can we move this into another PR? (I would suggest you opening it), because it's still not clear whether the main target of the current PR is actually desired or not.

And TBH, I don't have the resources to perform a proper benchmark, nor do I have good other arguments why the proposed style is better (for example more readable, more maintainable, etc).

@pcanal

Passing by const-reference can mean two constructions (once on the caller, once in the ctor using the TString copy ctor). Instead passing by value and using move semantics in the ctor can reduce this to one construction on the caller and one move in the ctor. As well simplify default constructor by defaulting it. Co-Authored-By: @pcanal

ChristianTackeGSI requested review from dpiparo and pcanal as code owners May 9, 2024 12:17

ChristianTackeGSI commented May 9, 2024

View reviewed changes

core/base/inc/TNamed.h Outdated Show resolved Hide resolved

ChristianTackeGSI force-pushed the pr/tstring_by_value branch from e11f5e2 to 03038d2 Compare May 9, 2024 15:38

pcanal reviewed May 9, 2024

View reviewed changes

dpiparo assigned pcanal May 10, 2024

guitargeek closed this Jun 3, 2024

guitargeek reopened this Jun 3, 2024

ChristianTackeGSI force-pushed the pr/tstring_by_value branch from 03038d2 to fbf8dee Compare June 4, 2024 16:11

pcanal reviewed Jun 5, 2024

View reviewed changes

pcanal requested changes Jun 5, 2024

View reviewed changes

pcanal reviewed Jun 5, 2024

View reviewed changes

core/base/inc/TNamed.h Outdated Show resolved Hide resolved

ChristianTackeGSI force-pushed the pr/tstring_by_value branch from fbf8dee to 2f9b6c8 Compare June 6, 2024 08:19

ChristianTackeGSI force-pushed the pr/tstring_by_value branch from 2f9b6c8 to 2bafc6e Compare June 6, 2024 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Pass TString by value to TNamed ctor #15469

[core] Pass TString by value to TNamed ctor #15469

ChristianTackeGSI commented May 9, 2024

github-actions bot commented May 9, 2024 •

edited

pcanal May 9, 2024

ChristianTackeGSI May 10, 2024

silverweed May 10, 2024

pcanal May 10, 2024

pcanal May 10, 2024 •

edited

ChristianTackeGSI commented Jun 4, 2024

guitargeek commented Jun 4, 2024

guitargeek commented Jun 4, 2024

ChristianTackeGSI commented Jun 4, 2024

pcanal commented Jun 4, 2024

pcanal commented Jun 4, 2024

pcanal commented Jun 4, 2024

pcanal Jun 5, 2024

pcanal left a comment

ChristianTackeGSI commented Jun 5, 2024

pcanal commented Jun 5, 2024

pcanal commented Jun 5, 2024 •

edited

ChristianTackeGSI commented Jun 5, 2024

pcanal commented Jun 5, 2024

ChristianTackeGSI commented Jun 6, 2024

- TNamed(TString name, TString title) : fName(std::move(name)), fTitle(std::move(title)) {}
+ TNamed(const &TString name, const &TString title) : fName(name), fTitle(title) {}
+ TNamed(TString &&name, const TString &title) : fName(std::move(name)), fTitle(title) {}
+ TNamed(const TString &name, TString &&title) : fName(name), fTitle(std::move(title)) {}
+ TNamed(TString &&name, TString &&title) : fName(std::move(name)), fTitle(std::move(title)) {}

- TNamed() = default;
+ // The following is *not* the default implementation (i.e. do not use `= default`.
+ // Default implementation can do 'more' than expected. In this case it reset all the
+ // data member of `TNamed` and more importantly its base class `TObject`.
+ // TObject's implementation of the `IsOnHeap` bit requires the memory occupied
+ // by `TObject::fUniqueID` to *not* be reset between the execution of `TObject::operator new`
+ // and the `TObject` constructor (Finding the magic pattern there is how we can determine
+ // that the object was allocated on the heap.
+ TNamed() : fName(), fTitle() { }

[core] Pass TString by value to TNamed ctor #15469

Are you sure you want to change the base?

[core] Pass TString by value to TNamed ctor #15469

Conversation

ChristianTackeGSI commented May 9, 2024

This Pull request:

Checklist:

github-actions bot commented May 9, 2024 • edited

Test Results

pcanal May 9, 2024

Choose a reason for hiding this comment

ChristianTackeGSI May 10, 2024

Choose a reason for hiding this comment

silverweed May 10, 2024

Choose a reason for hiding this comment

pcanal May 10, 2024

Choose a reason for hiding this comment

pcanal May 10, 2024 • edited

Choose a reason for hiding this comment

ChristianTackeGSI commented Jun 4, 2024

guitargeek commented Jun 4, 2024

guitargeek commented Jun 4, 2024

ChristianTackeGSI commented Jun 4, 2024

pcanal commented Jun 4, 2024

pcanal commented Jun 4, 2024

pcanal commented Jun 4, 2024

pcanal Jun 5, 2024

Choose a reason for hiding this comment

pcanal left a comment

Choose a reason for hiding this comment

ChristianTackeGSI commented Jun 5, 2024

pcanal commented Jun 5, 2024

pcanal commented Jun 5, 2024 • edited

ChristianTackeGSI commented Jun 5, 2024

pcanal commented Jun 5, 2024

ChristianTackeGSI commented Jun 6, 2024

github-actions bot commented May 9, 2024 •

edited

pcanal May 10, 2024 •

edited

pcanal commented Jun 5, 2024 •

edited