make get parent pointer consistent #974

Yurlungur · 2023-11-08T17:38:39Z

PR Summary

For the longest time now, MeshData::GetParentPointer has returned a Mesh* object, while MeshBlockData::GetParentPointer has returned a std::shared_ptr<MeshBlock>. This inconsistency is annoying to deal with in templated code that uses both levels of the abstraction hierarchy. It's also bad practice to pass around std::shared_ptr<T>& everywhere, which this return value encourages.

In this PR I Make MeshBlockData::GetParentPointer return a MeshBlock*. This required a few things:

To walk around the weak_ptr logic used for MeshBlockData, I needed to add a funciton that still returned a shared pointer. So there are now two underlying fundamental functions: GetBlockSharedPointer and GetBlockPointer.
I needed change all of the code that calls either GetBlockPointer or GetParentPointer to expect a MeshBlock* instead of a std::shared_ptr<MeshBlock>. The main places this showed up are in boundary comms.

This is a very small change that touches a lot of code, but it should be a quick review. @lroberts36 I mostly touched your code, so I think it's important you review this one.

Note that this may be a breaking change downstream, but the fix would be as simple as find-replace std::shared_ptr<MeshBlock> with MeshBlock* and maybe not even that if you're using auto.

PR Checklist

Code passes cpplint
New features are documented.
Adds a test for any bugs fixed. Adds tests for new features.
Code is formatted
Changes are summarized in CHANGELOG.md
CI has been triggered on Darwin for performance regression tests.
Docs build
(@lanl.gov employees) Update copyright on changed files

…turn a raw pointer, making it look the same as with MeshData.

lroberts36

This LGTM, I completely support switching to raw pointers where functions don't need to take ownership. Thanks for going through and making those changes, I am not sure why I ended up going with smart pointers in the boundary stuff in the first place.

src/bvals/comms/build_boundary_buffers.cpp

pgrete · 2023-11-10T20:40:06Z

Given that this is potentially API breaking, should we wait for the next release or should we already get this in (and I'll update the changelog when I make the release)?

Yurlungur · 2023-11-12T20:05:00Z

Given that this is potentially API breaking, should we wait for the next release or should we already get this in (and I'll update the changelog when I make the release)?

Ah good point. I say make a release first then this goes in immediately after release.

brryan · 2023-11-13T15:22:20Z

Since this is a breaking change dealing with how we manage pointers (and removing shared_ptrs from interfaces), is it worth considering whether we should remove the usage of weak_ptr entirely in this PR? As far as I'm aware we're always in the situation @lroberts36 described "where functions don't need to take ownership." We have about 25 std::weak_ptr::lock()'s in the code and I'm not sure they're contributing anything.

Yurlungur · 2023-11-13T15:47:33Z

Since this is a breaking change dealing with how we manage pointers (and removing shared_ptrs from interfaces), is it worth considering whether we should remove the usage of weak_ptr entirely in this PR? As far as I'm aware we're always in the situation @lroberts36 described "where functions don't need to take ownership." We have about 25 std::weak_ptr::lock()'s in the code and I'm not sure they're contributing anything.

IMO the weak pointers don't buy us a whole lot, but they're more defensible in the internals of the code. The main place they're used is as back-pointers for objects owned by MeshBlock, such as bvals and variable. I think in user code, those back-pointers are rarely used. The weak pointers buy us (technically) some memory safety as they're non-cyclical but reference counted. They cost us a little user-invisible complexity.

I'm fine keeping them or removing them---I don't feel strongly. That said, since this PR already works, I'd rather get it in and then remove the weak_ptrs in a separate PR, rather than letting this one languish and potentially diverge form develop.

brryan · 2023-11-13T18:13:55Z

IMO the weak pointers don't buy us a whole lot, but they're more defensible in the internals of the code. The main place they're used is as back-pointers for objects owned by MeshBlock, such as bvals and variable. I think in user code, those back-pointers are rarely used. The weak pointers buy us (technically) some memory safety as they're non-cyclical but reference counted. They cost us a little user-invisible complexity.

I'm fine keeping them or removing them---I don't feel strongly. That said, since this PR already works, I'd rather get it in and then remove the weak_ptrs in a separate PR, rather than letting this one languish and potentially diverge form develop.

Thanks for clarifying. Given your points that the weak pointers are internal and that they do protect against cyclic dependencies, I'm fine to defer thinking about our weak pointer usage to later/never.

brryan

Two non-blocking comments you're welcome to ignore, just mentioning them in case you think they are worth the effort to make the changes.

brryan · 2023-11-13T21:57:36Z

src/interface/meshblock_data.hpp

@@ -97,10 +97,10 @@ class MeshBlockData {
    SetBlockPointer(other.get());
  }
  void SetBlockPointer(const MeshBlockData<T> &other) {
-    pmy_block = other.GetBlockPointer();
+    pmy_block = other.GetBlockSharedPointer();


Since pointers and shared pointers are coexisting more closely in some cases clarity might be improved by naming shared points spmy_* and naming pointers pmy_*. I don't think this is blocking but I'm just bringing it up in case you agree.

brryan · 2023-11-13T21:59:19Z

src/mesh/amr_loadbalance.cpp

@@ -751,7 +751,7 @@ void Mesh::RedistributeAndRefineMeshBlocks(ParameterInput *pin, ApplicationInput
      auto pmb = FindMeshBlock(on);
      for (auto &var : pmb->vars_cc_) {
        restriction_cache.RegisterRegionHost(
-            irestrict++, ProResInfo::GetInteriorRestrict(pmb, NeighborBlock(), var),
+            irestrict++, ProResInfo::GetInteriorRestrict(pmb.get(), NeighborBlock(), var),


Would FindMeshBlock returning a pointer rather than a shared pointer be more in the spirit of this PR?

Yurlungur · 2023-11-21T15:54:18Z

Now we're post release, I'm clicking the button

jonahm-LANL added 3 commits November 7, 2023 18:15

converted GetParentPointer and GetBlockPointer in MeshBlockData to re…

f8f9e09

…turn a raw pointer, making it look the same as with MeshData.

merge with multigrid

679616a

formatting

100f13b

Yurlungur added refactor An improvement to existing code. breaks-downstream labels Nov 8, 2023

Yurlungur requested review from BenWibking, brryan, forrestglines, jdolence, lroberts36, pgrete and pdmullen November 8, 2023 17:38

Yurlungur self-assigned this Nov 8, 2023

jonahm-LANL added 2 commits November 8, 2023 10:39

changelog

be4af0a

copyrights

ef9463f

lroberts36 approved these changes Nov 8, 2023

View reviewed changes

src/bvals/comms/build_boundary_buffers.cpp Show resolved Hide resolved

Merge branch 'develop' into jmm/make-get-parent-pointer-consistent

eb29ede

brryan approved these changes Nov 13, 2023

View reviewed changes

Yurlungur added 2 commits November 15, 2023 15:27

Merge branch 'develop' into jmm/make-get-parent-pointer-consistent

fcf897d

Merge branch 'develop' into jmm/make-get-parent-pointer-consistent

0e498ee

Yurlungur enabled auto-merge November 21, 2023 15:54

Yurlungur merged commit 0249bde into develop Nov 21, 2023
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make get parent pointer consistent #974

make get parent pointer consistent #974

Yurlungur commented Nov 8, 2023 •

edited

Loading

lroberts36 left a comment

pgrete commented Nov 10, 2023

Yurlungur commented Nov 12, 2023

brryan commented Nov 13, 2023

Yurlungur commented Nov 13, 2023

brryan commented Nov 13, 2023

brryan left a comment

brryan Nov 13, 2023

brryan Nov 13, 2023

Yurlungur commented Nov 21, 2023

make get parent pointer consistent #974

make get parent pointer consistent #974

Conversation

Yurlungur commented Nov 8, 2023 • edited Loading

PR Summary

PR Checklist

lroberts36 left a comment

Choose a reason for hiding this comment

pgrete commented Nov 10, 2023

Yurlungur commented Nov 12, 2023

brryan commented Nov 13, 2023

Yurlungur commented Nov 13, 2023

brryan commented Nov 13, 2023

brryan left a comment

Choose a reason for hiding this comment

brryan Nov 13, 2023

Choose a reason for hiding this comment

brryan Nov 13, 2023

Choose a reason for hiding this comment

Yurlungur commented Nov 21, 2023

Yurlungur commented Nov 8, 2023 •

edited

Loading