Allow multiple callbacks on same component by knelli2 · Pull Request #6334 · sxs-collaboration/spectre

knelli2 · 2024-10-15T23:59:25Z

Proposed changes

It is possible for multiple unique callbacks to be registered to the same parallel component (size control system does this). Previously, we only took the first callback and discarded the rest (because the elements would register multiple identical callbacks and we only needed one). Now we still discard identical callbacks, but keep unique callbacks on the same component.

This was found while debugging the nodegroup implementation of our algorithm, but I believe this is also a bug on develop. I guess we just have been lucky and haven't hit this so far.

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 · 2024-10-16T02:41:53Z

src/Parallel/Callback.hpp

+   * \brief Returns if this callback is equal to the one passed in.
+   *
+   * \details If the `std::optional` doesn't have a value, then the pointer
+   * cast between the two callbacks failed. If it does have a value, then the


There's some sort of context missing for this documentation. There doesn't seem to be any mention of pointers or casts anywhere else.

wthrowe · 2024-10-16T02:43:04Z

src/Parallel/Callback.hpp

+  std::optional<bool> is_equal_to(const Callback& rhs) const override {
+    const auto* downcast_ptr = dynamic_cast<const SimpleActionCallback*>(&rhs);
+    if (downcast_ptr == nullptr) {
+      return std::nullopt;


Why this distinction instead of just returning false?

It was particularly useful in debugging to know if the callbacks were different types, or if they were the same but had different arguments. At some point, I would like to put debugging info/prints into the cache and this kind of info would be useful, but that's beyond this PR so I just left the return types as optional<bool> for now

Just return a bool. If you want to know if the types are the same for debugging you can check that directly without complicating the public interface.

wthrowe · 2024-10-16T02:47:50Z

tests/Unit/Parallel/Test_Callback.cpp

+    const std::optional<bool> callback_2_equal_to_vec_end =
+        callback_2.is_equal_to(*callbacks.back());
+    SPECTRE_PARALLEL_REQUIRE(callback_2_equal_to_vec_end.has_value());
+    SPECTRE_PARALLEL_REQUIRE_FALSE(callback_2_equal_to_vec_end.value());


[optional] callback_2_equal_to_vec_end == std::optional{false} seems clearer to me than these two lines.

wthrowe · 2024-10-16T02:49:56Z

src/Parallel/Callback.hpp

 namespace Parallel {
+namespace detail {
+// Not all tuple arguments are guaranteed to have operator==, so we check the
+// ones we can.


I'm a bit worried about silently ignoring entries. What types typically occur that cause problems?

The problematic types were proxies and I didn't want to deal with how to evaluate equivalence of those

OK. charmplusplus/charm#3848 is probably the real fix, for what it's worth, but we can't wait for that.

wthrowe · 2024-10-21T18:01:09Z

Looks good. Squash.

knelli2 · 2024-10-21T19:03:49Z

@wthrowe I realized I inadvertently added a bug related to removing an unnecessary callback if the cache item was mutated and is now ready. I added a new commit and a new fixup, so let me know if you need more of a explanation, or if the code is enough.

wthrowe

Good catch. Test isn't compiling, though.

wthrowe · 2024-10-21T19:33:16Z

src/Parallel/GlobalCache.hpp

+      // array component id still has callbacks before trying to remove them.
+      if (callbacks.contains(array_component_id)) {
+        if (callbacks.at(array_component_id).empty()) {
+          callbacks.erase(array_component_id);


Check if the vector is empty after removing the element, instead of before.

wthrowe · 2024-10-21T19:35:01Z

src/Parallel/Callback.hpp


+  std::unique_ptr<Callback> get_clone() override {
+    return std::make_unique<SimpleActionCallback<SimpleAction, Proxy, Args...>>(
+        proxy_, args_);


[optional] Using the copy constructor is simpler. (Also several other places.)

nilsdeppe

Just a couple small things you can squash with everything else.

nilsdeppe · 2024-10-21T20:49:21Z

src/Parallel/GlobalCache.hpp

+          callbacks.erase(array_component_id);
+        } else {
+          // If this callback was a duplicate, we'll have to search through all
+          // callbacks to determine which to remove. If it wasn't a dupliate,


dupliate -> duplicate

nilsdeppe · 2024-10-21T20:54:17Z

src/Parallel/GlobalCache.hpp

+          auto& vec_callbacks = callbacks.at(array_component_id);
+          auto it_to_erase = vec_callbacks.begin();
+          for (; it_to_erase != vec_callbacks.end(); it_to_erase++) {
+            if ((*it_to_erase)->is_equal_to(*clone_of_optional_callback)) {
+              vec_callbacks.erase(it_to_erase);
+              break;
+            }
+          }


I think this can be done with

vec_callbacks.erase(std::find(vec_callbacks.begin(), vec_callbacks.end(), [&clone_of_optional_callback](const auto& t) { return t.is_equal_to(*clone_of_optional_callback); });

wthrowe · 2024-10-22T22:00:49Z

Looks good. Squash.

knelli2 added the bugfix label Oct 15, 2024

knelli2 requested review from nilsdeppe and wthrowe October 15, 2024 23:59

wthrowe requested changes Oct 16, 2024

View reviewed changes

wthrowe requested changes Oct 21, 2024

View reviewed changes

nilsdeppe requested changes Oct 21, 2024

View reviewed changes

knelli2 added 3 commits October 22, 2024 17:17

Add is_equal_to and name functions to Parallel::Callback

d97211a

Add get_clone to Parallel::Callback

4a47e03

Allow multiple parallel callbacks on same component

0f096a3

knelli2 force-pushed the cache_callback_fix branch from a77a380 to 0f096a3 Compare October 23, 2024 00:31

wthrowe approved these changes Oct 23, 2024

View reviewed changes

nilsdeppe approved these changes Oct 23, 2024

View reviewed changes

nilsdeppe merged commit 4e8ff38 into sxs-collaboration:develop Oct 23, 2024

Comments

Conversation

knelli2 commented Oct 15, 2024

Proposed changes

Upgrade instructions

Code review checklist

Further comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wthrowe commented Oct 21, 2024

Uh oh!

knelli2 commented Oct 21, 2024

Uh oh!

wthrowe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilsdeppe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wthrowe commented Oct 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants