Performance optimizations for shared secondary bank#4011
Conversation
jtramm
left a comment
There was a problem hiding this comment.
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.
jtramm
left a comment
There was a problem hiding this comment.
60% is a killer optimization indeed -- thanks for running the test!!
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
SharedArrayfollowed by a sort on the array to preserve reproducibility. I've made the following changes: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):

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:

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