Use charm inline method instead of eliding charm #3067

moxcodes · 2021-04-05T21:13:33Z

This allows the charm load measurements to track the inlined methods, which
enables useful measurements by projections, and should improve load balancing
performance.

I've elected to perform the post-processing of the charm-generated files by putting patch files in the repository and applying them with a script in cmake rather than (for instance), creating a regex replacement script. This is a bit more verbose in terms of the repo changes, but I think it's considerably safer. When the charm files change due to ci modifications or charm version changes, this should force us to regenerate the patch files, which is better because I'm not certain about how these files will evolve in the future.

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
major new feature if appropriate.

nilsdeppe

LGTM, just a few small things you can squash immediately :)

nilsdeppe · 2021-04-05T21:29:35Z

tools/charm_module_patches/src/Parallel/Algorithms/AlgorithmArray.def.h.patch

@@ -0,0 +1,49 @@
+Distributed under the MIT License.


I think we do the super consistent thing (there appears to be no sarcasm emoji? 😞 ) of CamelCasing nested directories (CharmModulePatches)

nilsdeppe · 2021-04-05T21:30:46Z

tools/patch_charm_modules.sh

@@ -0,0 +1,27 @@
+#!/usr/bin/env bash


[I'm a newbie with shell scripts] should this have something like a -e so that if there is an error in the script it propagates and the script returns the error?

Yes, there should be a -e somewhere, either on this line or as a set -e.

Because of limitations in how the #! line is parsed (by the kernel? I don't actually know.) you only get to pass one argument to the called program. Here that argument is "bash", so #!/usr/bin/env bash -e will fail (It will try to run the program bash -e.), but env has special handling for this and you can write #!/usr/bin/env -S bash -e and env will make it work.

nilsdeppe · 2021-04-05T21:32:21Z

src/Parallel/Algorithms/AlgorithmArray.ci

            initialization_items);

    template <typename Action, typename... Args>
-    entry void simple_action(std::tuple<Args...> & args);


I think you need to include the header file that holds Parallel::detail::max_inline_entry_methods_reached() above on line 5ish. Probably best to make sure that function is in its own file if it isn't already 🤷

It might also be worth placing a comment on line 26 with a short explanation that the inline would cause the stack to blow because of infinite recursion, however we use Parallel::detail::max_inline_entry_methods_reached() to limit the stack depth and avoid such a scenario. Thus allowing us to maximally inline code, while supporting load balancing and avoiding stack overflows.

kidder

Just some questions to help my understanding of what is being done here...

@wthrowe can you please look at the shell scripting in this PR

kidder · 2021-04-06T01:17:42Z

src/Parallel/Invoke.hpp

-        std::move(temporal_id), std::forward<ReceiveDataType>(receive_data),
-        enable_if_disabled);
-  }
+  proxy.template receive_data<ReceiveTag, std::decay_t<ReceiveDataType>>(


why do you need to specify the second template parameter now?

This is a frustrating result of the charm definition file changes when [inline] is included. For some reason, in one of the places it appears, it adds template parameters for all of the arguments, making the original two template arguments not inferrable unless explicitly specified.

ugh; oh well

kidder · 2021-04-06T01:18:05Z

tests/Unit/Parallel/Test_AlgorithmCore.cpp

                             TestAlgorithmArrayInstance{4});
    SPECTRE_PARALLEL_REQUIRE(db::get<CountActionsCalled>(box) == 13);
-    SPECTRE_PARALLEL_REQUIRE(db::get<Int1>(box) == 5);
+    SPECTRE_PARALLEL_REQUIRE(db::get<Int1>(box) == 10);


why did this change?

This tag counts the number of times that is_ready is called for this component. My understanding is that the count can be different because the execute_next_phase calls both start_phase and receive_data, so the count depends on the ordering of those messages. Previously, the test actually varied what the count will be if it is run in parallel, but was consistently 5 in serial.
Now, it is consistently 10 (always running an is_ready check that returns false before the receive_data), regardless of how many processors the test is run on. My understanding is that this is because the perform_algorithm call is also inline now, so in consistently calls the execute_next_phase operations in order.

kidder · 2021-04-06T01:24:30Z

tools/charm_module_patches/src/Parallel/Algorithms/AlgorithmArray.def.h.patch

+   ckCheck();
+   AlgorithmArray <ParallelComponent, SpectreArrayIndex>  *obj = ckLocal();
+-  if (obj) {
+  if (obj != nullptr and not Parallel::detail::max_inline_entry_methods_reached()) {


I am confused; above you removed a similar conditional but there was an else branch; what happens now when max_inline_entry_methods_reached() is true?

the charm generated functions already have a message-passing else (that's below the patch range) that executes when the object isn't on the current core. This just ensures that it also does the message-passing thing if it hits our inline cap.

okay, thanks

wthrowe

I've only reviewed the scripts.

wthrowe · 2021-04-06T17:16:50Z

tools/patch_charm_modules.sh

@@ -0,0 +1,27 @@
+#!/usr/bin/env bash


Yes, there should be a -e somewhere, either on this line or as a set -e.

Because of limitations in how the #! line is parsed (by the kernel? I don't actually know.) you only get to pass one argument to the called program. Here that argument is "bash", so #!/usr/bin/env bash -e will fail (It will try to run the program bash -e.), but env has special handling for this and you can write #!/usr/bin/env -S bash -e and env will make it work.

wthrowe · 2021-04-06T17:22:37Z

tools/patch_charm_modules.sh

+if [ "$#" -ne 4 ]; then
+    echo "Usage: patch_charm_modules.sh module_name path_to_current_source_dir\
+ path_to_source_root_dir path_to_current_build_dir"
+    exit


wthrowe · 2021-04-06T17:33:42Z

tools/patch_charm_modules.sh

+    exit
+fi
+
+PATCH_PREFIX=`echo "$2" | sed -En 's@'$3'@'$3'/tools/charm_module_patches@p'`


PATCH_PREFIX=$3/tools/charm_module_patches${2#"$3"}
In words, that's: root directory followed by "/tools/charm_module_patches" followed by (current directory with root directory removed from the beginning).

moxcodes · 2021-04-06T18:01:17Z

fixups posted.
Cheers!

wthrowe · 2021-04-06T18:35:17Z

My requests look good.

kidder

you have my permission to squash

nilsdeppe

LGTM, please squash. CI says:

MaxInlineMethodsReached.hpp should be added to ./src/Parallel/CMakeLists.txt

moxcodes · 2021-04-06T22:21:20Z

Squashed.
I had to revert the second call to receive_data in Invoke.hpp to support the silly business of whether or not the second template parameter is correctly specified. In doing so I used if constexpr instead of Requires:

template <typename ReceiveTag, typename Proxy, typename ReceiveDataType>
void receive_data(Proxy&& proxy, typename ReceiveTag::temporal_id temporal_id,
                  ReceiveDataType&& receive_data,
                  const bool enable_if_disabled = false) noexcept {
  if constexpr (detail::has_ckLocal_method<std::decay_t<Proxy>>::value) {
    proxy.template receive_data<ReceiveTag, std::decay_t<ReceiveDataType>>(
        std::move(temporal_id), std::forward<ReceiveDataType>(receive_data),
        enable_if_disabled);
  } else {
    proxy.template receive_data<ReceiveTag>(
        std::move(temporal_id), std::forward<ReceiveDataType>(receive_data),
        enable_if_disabled);
  }
}

Cheers!

This allows the charm load measurements to track the inlined methods, which enables useful measurements by projections, and should improve load balancing performance

moxcodes requested a review from nilsdeppe April 5, 2021 21:13

nilsdeppe reviewed Apr 5, 2021

View reviewed changes

kidder reviewed Apr 6, 2021

View reviewed changes

wthrowe requested changes Apr 6, 2021

View reviewed changes

kidder reviewed Apr 6, 2021

View reviewed changes

nilsdeppe reviewed Apr 6, 2021

View reviewed changes

moxcodes force-pushed the use_charm_inline branch from 3fe7389 to 2e0d988 Compare April 6, 2021 22:21

Use charm inline method instead of eliding charm

fd2b8a8

This allows the charm load measurements to track the inlined methods, which enables useful measurements by projections, and should improve load balancing performance

moxcodes force-pushed the use_charm_inline branch from 2e0d988 to fd2b8a8 Compare April 6, 2021 22:21

nilsdeppe approved these changes Apr 6, 2021

View reviewed changes

kidder approved these changes Apr 7, 2021

View reviewed changes

wthrowe approved these changes Apr 8, 2021

View reviewed changes

wthrowe merged commit a4ac9d0 into sxs-collaboration:develop Apr 8, 2021

Use charm inline method instead of eliding charm #3067

Use charm inline method instead of eliding charm #3067

Uh oh!

Conversation

moxcodes commented Apr 5, 2021

Upgrade instructions

Code review checklist

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidder 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!

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 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!

Choose a reason for hiding this comment

Uh oh!

moxcodes commented Apr 6, 2021

Uh oh!

wthrowe commented Apr 6, 2021

Uh oh!

kidder left a comment

Choose a reason for hiding this comment

Uh oh!

nilsdeppe left a comment

Choose a reason for hiding this comment

Uh oh!

moxcodes commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants