Replace Core State counter synchronization with host functors or device kernel calls#2242
Replace Core State counter synchronization with host functors or device kernel calls#2242LSchwiebert wants to merge 48 commits into
Conversation
…ert/celeritas into CoreStateCounters-track
…ebert/celeritas into CoreStateCounters-optical
…ebert/celeritas into CoreStateCounters-kernels
Test summary 5 905 files 9 496 suites 7m 58s ⏱️ Results for commit 5a5eb8b. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2242 +/- ##
===========================================
- Coverage 86.87% 85.21% -1.66%
===========================================
Files 1331 1298 -33
Lines 42220 40671 -1549
Branches 13000 10595 -2405
===========================================
- Hits 36677 34657 -2020
- Misses 4342 4408 +66
- Partials 1201 1606 +405
🚀 New features to boost your workflow:
|
amandalund
left a comment
There was a problem hiding this comment.
Thanks @LSchwiebert! The small change set is definitely helpful for these types of algorithmic updates, and this looks like the right path forward.
For the particular case you started with (setting the number of pending tracks in the core counters when primaries are inserted) I'm actually not sure we need to update that value at all. That's my fault: in the optical loop, which uses the same CoreStateCounters struct, we do need to keep track of the number of pending tracks, so I think I added it here for completeness since at the time it was free. I don't think it's actually needed in the current stepping algorithm: it's just cleared again almost immediately.
I think it would help me a lot (and you might have already done some of this on your own) to go through each case where we're currently syncing the counters and figure out how we plan to make each one asynchronous (and confirm they are necessary). That should also give us an idea of whether we'll need a special kernel for each case or whether there's a more general, reusable approach.
|
|
||
| //---------------------------------------------------------------------------// | ||
| /*! | ||
| * Launch a kernel for the wrapped executor with a specific number of threads. |
There was a problem hiding this comment.
You should be able to do this using the KernelLauncher in corecel/sys instead of the ActionLauncher.
There was a problem hiding this comment.
Thanks. I'll switch to that.
|
Thanks for taking a look at my PR. I'm going to remove that counter update and confirm that the tests are passing. Assuming that's the case, and I'm sure it is, I'll revert that code but save it as a template for the other similar changes. I did go through and make a list of the places in the code where we had a sync_get_counters() call and tried to figure out which ones needed to be changed and which ones didn't. I didn't check the sync_put_counters() calls, since they are paired with a sync_get_counters() call if we are changing any counters. Or they are being used to initialize the counters and likely need to remain. However, I didn't make a note of what changes need to be made, just where they need to be made. So, I'll go back and add that information and then share that document. |
|
That would be awesome! If you wanted to share a google doc or something ahead of time I'd be more than happy to sketch them out together (since this is mostly for my benefit :) ). Probably a lot of them will be straightforward like the example you implemented here, but there were a couple places where it wasn't obvious to me what the best approach was. |
|
Closing this for now. Will reopen when the other changes are done -- or at least enough that we want to merge into the code base. |
This PR replaces the synchronized Core State counter getter and setter functions with device kernel calls or host functors. These getter and setter functions were introduced in PR #2177 and #2208, which moved these counters to the device. The objective is to remove synchronization between the host and device when running an event.
Currently, this has code to revise only one synchronization -- the synchronization in the Stepper::operator()(SpanConstPrimary primaries) function -- with a functionally-equivalent executor functor. Once everyone is happy with the solution used here, it will be replicated elsewhere to replace other synchronizations. Just trying to reduce the burden of reviewing similar changes.