Skip to content

Add non-copying Variables-VectorImpl conversion#7086

Merged
nilsdeppe merged 4 commits intosxs-collaboration:developfrom
wthrowe:variables_datavector
Mar 11, 2026
Merged

Add non-copying Variables-VectorImpl conversion#7086
nilsdeppe merged 4 commits intosxs-collaboration:developfrom
wthrowe:variables_datavector

Conversation

@wthrowe
Copy link
Member

@wthrowe wthrowe commented Feb 24, 2026

Proposed changes

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@wthrowe
Copy link
Member Author

wthrowe commented Feb 25, 2026

I am at a loss on the CI linker errors: error: undefined reference to 'std::unique_ptr<double [], std::default_delete<double []> >::unique_ptr<std::default_delete<double []>, void>()'. This is obviously a compiler bug, since that function is defined inline in its only declaration, but I don't see anything different that seems significant between the failing file tests/Unit/Evolution/Systems/GrMhd/ValenciaDivClean/FiniteDifference/Test_MonotonicityPreserving5.cpp and similar code that doesn't seem to be giving errors. I have tried to reproduce the problem locally with gcc 11.5.0 (which is as close as my distro has to the failing version), and while I haven't gotten it to link because I would need to recompile all the C++ dependencies for the older libstdc++, the Test_MonotonicityPreserving5.cpp.o object does not have any symbols related to std::unique_ptr<double[]>, so I don't think I would see the problem if I got the dependencies worked out.

@wthrowe
Copy link
Member Author

wthrowe commented Feb 27, 2026

I have managed to reproduce this in codespaces. To correct my previous comment: this does affect multiple files, it's just that the linker gives up after the first failing one. My best guess as to what's going on at this point is that old gcc versions are miscompiling some piece of code (something with large TaggedTuples, possibly the Tensors in Variables) by failing to either inline or emit this constructor, and just happened to decide to emit it for the Variables data and hide the issue. The best workaround I've found is to add a completely unused unique_ptr in the Variables class, which the compiler apparently decides to emit a default constructor for.

@nilsdeppe nilsdeppe self-requested a review March 4, 2026 03:49
nilsdeppe
nilsdeppe requested changes Mar 4, 2026
View reviewed changes
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can squash

tests/Unit/DataStructures/Test_Variables.cpp Outdated
CHECK(transferred_back.number_of_grid_points() == num_points);
CHECK(transferred_back.size() ==
num_points * Vars::number_of_independent_components);
CHECK(get<TestHelpers::Tags::Scalar<VectorType>>(vars) == orig_scalar);
Copy link
Member

@nilsdeppe nilsdeppe Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm confused. Shouldn't this fail because it was moved from on line 1368? I naively would've guessed this is transferred_back instead of vars. Does that mean release is not reseting the pointers inside Tensors? If the code is correct, I think adding a comment as to why would be good.

Copy link
Member Author

@wthrowe wthrowe Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a copy-paste error. I have corrected it and added a new commit fixing the bug that caused the test to incorrectly pass ("Fix bugs when reducing a Variables's size to 0").

tests/Unit/DataStructures/Test_Variables.cpp Outdated
CHECK(transferred_back.data() != transferred_data);
CHECK(transferred_back.number_of_grid_points() == 1);
CHECK(transferred_back.size() == Vars::number_of_independent_components);
CHECK(get<TestHelpers::Tags::Scalar<VectorType>>(vars) == orig_scalar);
Copy link
Member

@nilsdeppe nilsdeppe Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question as above.

@wthrowe wthrowe force-pushed the variables_datavector branch from 4c45e88 to 30631b9 Compare March 4, 2026 22:49
wthrowe added 4 commits March 9, 2026 15:20
@wthrowe
Change storage of Variables to vector_type
fb31286
@wthrowe
Add non-copying Variables-VectorImpl conversion
8b55463 
Copies (and allocates in one direction) for single-point variables
because of the Variables small-vector optimization, but should
otherwise be effectively free.
@wthrowe
Add into_math_wrapper_type for owning type-erasure
eb387a8
@wthrowe
Fix bugs when reducing a Variables's size to 0
f8fd40a
@wthrowe wthrowe force-pushed the variables_datavector branch from 30631b9 to f8fd40a Compare March 9, 2026 19:22
@wthrowe
Copy link
Member Author

wthrowe commented Mar 9, 2026

Rebased to pick up CI fixes.

@kidder kidder enabled auto-merge March 9, 2026 20:23
@nilsdeppe nilsdeppe disabled auto-merge March 11, 2026 03:36
@nilsdeppe nilsdeppe merged commit d8532c3 into sxs-collaboration:develop Mar 11, 2026
22 of 24 checks passed
@wthrowe wthrowe mentioned this pull request Mar 12, 2026
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nilsdeppe nilsdeppe nilsdeppe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wthrowe @nilsdeppe