Trilinos icon indicating copy to clipboard operation
Trilinos copied to clipboard

Teuchos: Move semantics for `Teuchos::any` and `Teuchos::ParameterEntry`.

Open romintomasetti opened this issue 2 years ago • 8 comments

Enhancement

teuchos @bartlettroscoe

It seems that some Teuchos classes like any and ParameterEntry miss move semantics (constructor and assignment).

  • Teuchos::any can be used anywhere to embed any type of object, so the performance gain of the move semantic is clear.
  • Teuchos::ParameterEntry can contain huge sub-lists, and being able to move them instead of copying would be great. For instance, depending on the size of the std::string stored in a parameter list, copying the whole sub-list might be costly (c.f. Small String Optimizations)

This might potentially be related to:

  • https://github.com/trilinos/Trilinos/issues/6031

Here is a patch: teuchos-fix-add-missing-move-constructors.patch.txt

The patch has been fully tested (we run Teuchos, Tpetra, Stokhos tests on OpenMP, Cuda and HIP).

@maartenarnst

romintomasetti avatar Jan 19 '23 14:01 romintomasetti

Teuchos::any should probably be deprecated in favor of std::any now that trilinos requires C++17.

rppawlo avatar Jan 19 '23 15:01 rppawlo

@rppawlo That would be even better! But it would require more effort (maybe) :smile:

romintomasetti avatar Jan 19 '23 15:01 romintomasetti

@romintomasetti, would you be open to posting a PR for these changes? (The move to std::any can come as a follow-on PR.)

bartlettroscoe avatar Jan 19 '23 16:01 bartlettroscoe

@bartlettroscoe Here you are: https://github.com/trilinos/Trilinos/pull/11484 :smile:

romintomasetti avatar Jan 19 '23 17:01 romintomasetti

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

github-actions[bot] avatar Jan 20 '24 12:01 github-actions[bot]

@rppawlo @bartlettroscoe Any plan to deprecate (or remove) Teuchos::any ?

romintomasetti avatar Feb 10 '24 12:02 romintomasetti

@romintomasetti - we definitely should remove Teuchos::any now that we require c++17. No one is working on it as far as I know. If you wanted to remove it, feel free. Otherwise, we can bring it up at the weekly trilinos meeting.

rppawlo avatar Feb 12 '24 16:02 rppawlo

I think it would be nice to raise this as soon as possible in a Teuchos meeting.

I personally don't have enough time resources to do it, but I quickly looked at the feasibility.

It seems the only thing that is really far from std::any is https://github.com/trilinos/Trilinos/blob/f1aef01a78957e9a04965949c0d5a97fe182e93b/packages/teuchos/core/src/Teuchos_any.hpp#L316

Otherwise I don't see any major blocker.

romintomasetti avatar Feb 12 '24 18:02 romintomasetti