Skip to content

Performance optimizations for shared secondary bank#4011

Merged
jtramm merged 5 commits into
openmc-dev:developfrom
paulromano:shared-sec-perf-opt2
Jul 17, 2026
Merged

Performance optimizations for shared secondary bank#4011
jtramm merged 5 commits into
openmc-dev:developfrom
paulromano:shared-sec-perf-opt2

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

This PR is a follow-on to #3995 with further optimizations I've made for when the shared secondary bank is turned on. The optimizations are focused on how thread-local banks are collected into the shared secondary bank. Currently, this involves a critical section to drain the thread-local banks into a single SharedArray followed by a sort on the array to preserve reproducibility. I've made the following changes:

  • First, I avoided the critical section on the drain of thread-local banks by pre-allocating the full bank, which then allows us to copy data into the appropriate offset from each thread using an exclusive scan on the progeny per particle.
  • Once I did that, I noted that we can precompute the index into the sorted array using the parent_id, progeny_id, and the progeny offsets, which allows us to entirely avoid the sort on the shared secondary bank.
  • After a generation's read bank has been used, its storage is no longer needed which means we can reuse that storage as the next write bank. This avoids extra memory allocations when the next secondary generation is no larger than the current one.

Performance Profiling

As I mentioned in #3995, these optimizations arose when I was observing poor performance on a D1S simulation using weight windows. To share some profiles as I was going through these optimizations. Here is the starting point (before #3995):
Before optimizations

You can see a huge block from the memory allocation of the particle cross section caches (addressed in #3995) but also some other inefficiences due to the allocation/sorting of the shared secondary bank itself (addressed here). After the optimizations from #3995 and this PR, this is what the profile looks like now:
After optimizaitons

At this point, the major "overhead" from the shared secondary mode is computing random number seeds (future_seed_coefficients) and I don't think there's much we can do about that.

Overall for this problem, the performance with the shared secondary bank improved by about 2.3× with both sets of optimizations, and whereas before it was about 37% of the performance with a local secondary bank, now it reaches 77% of the performance with a local secondary bank (turns out for this problem the load balancing between threads is not bad enough for shared secondary to give better performance than local).

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 18) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@paulromano
paulromano requested a review from jtramm July 11, 2026 15:43

@jtramm jtramm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on continuing to optimize the shared secondary performance @paulromano!

One concern I have is that the performance testing validation for this PR was done inclusive of the changes from #3995 as well, so it's unclear how big of an impact the changes in this PR make by themselves. It does look like it should be a performance improvement given that there are definitely fewer memory operations, though given it's still doing the same complexity O(n) scan-and-sort, I wouldn't necessarily expect it to guarantee an observable effect. The downside is that we went from one shared sorting function for both fission bank + secondary bank with sort_bank(), to now having a separate bespoke sorting function for the secondary bank with collect_sorted_history_secondary_banks(), so it does seem like a tangible increase in code complexity.

I did some testing on my own with a weight window problem and wasn't able to see any significant performance difference compared to the current develop branch (which now has #3995 merged). Some of the develop runs were actually slightly faster. It's definitely still very possible that your test case may show a bigger impact, or perhaps running on a larger computer would as well (I was just testing on my laptop). In any event, I think it would be helpful if you were able to re-run your D1S test case again with this PR vs. current develop to confirm you're really seeing speedup on your problem. Or, if you have another easier test case to run that shows speedup, that would be totally fine as well.

I just want to be 100% sure we're not expanding code complexity without a demonstrated speedup on some use case. That said, I could also see the argument that this implementation could be considered a little cleaner as more of the secondary stuff is contained in a single function, but I do think that the loss of a shared sorting function is a clear downside.

Comment thread src/simulation.cpp Outdated
@paulromano

Copy link
Copy Markdown
Contributor Author

I just re-ran the D1S problem that I was using a test case and the develop branch gives me ~255 particles/sec and this branch gives me ~410 particles/sec, so a pretty significant performance improvement. I was hoping to show that convincingly with profiles and while it's not glaringly obvious, the profiles do illustrate the areas that are eliminated. Here is the profile from the develop branch:
develop
and for this PR:
pr4011

The blocks that are eliminated are openmc::SharedArray::thread_unsafe_append and openmc::sort_bank.

@jtramm jtramm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

60% is a killer optimization indeed -- thanks for running the test!!

@jtramm
jtramm enabled auto-merge (squash) July 17, 2026 00:58
@jtramm
jtramm merged commit f1fb672 into openmc-dev:develop Jul 17, 2026
18 of 21 checks passed
@paulromano
paulromano deleted the shared-sec-perf-opt2 branch July 17, 2026 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants