-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Random Ray Source Region Refactor #3288
Open
jtramm
wants to merge
18
commits into
openmc-dev:develop
Choose a base branch
from
jtramm:source_region_refactor2
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+832
−460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR changes the way source region data is stored in OpenMC. It is generally a refactor -- it does not change any program behavior or output, save for one bug fix (as described later on below).
Previously, source region data was stored in "Structure of Arrays" (SoA) format within
FlatSourceDomain
orLinearSourceDomain
objects, with a serialized array constituting each variable (with values for each source region and energy group within each array). This is the most efficient storage format for this use case, and allows for easy contiguous looping over all elements of each variable type.This has worked great to date, but the downside is that this storage technique is not very friendly to the abstraction of a single source region object. Having a single object that holds only the info for a single source region, e.g., a
SourceRegion
class,will be useful in upcoming work. Additionally, the current SoA format is not very amenable to dynamically appending source regions, which will be desirable for upcoming features.In this PR, a
SourceRegion
class is made which contains all data for a single source region (including all energy groups - making this object also a SoA). Naively we could just store this object in a vector to represent all source regions, as an Array of Structures of Arrays (AoSoA). However, single variables (like the scalar flux) would no longer be contiguous in memory, and would also involve another layer of indirection. I tried this AoSoA storage scheme out and found that it was about 30% slower on 2D C5G7 as compared to the original SoA.Thus, to allow for both abstraction and efficiency, I have introduced a second new class
SourceRegionContainer
that essentially retains the original SoA format, but emits accessors that abstract the access to the underlying data. Additionally, it can handle appendingSourceRegion
objects dynamically, with the contents distributed automatically to the various vectors. It can also handle reduction of itself.Another question is why we didn't just leave the SoA fields in the
FlatSourceDomain
andLinearSourceDomain
classes and add methods to them for appending the newSourceRegion
object. This would have worked, but I like the new hierarchy as it allows for theSourceRegionContainer
class to really just store source region-specific information only, and only have methods that have to do with the storage of that data. The higher levelFlatSourceDomain
classes contain additional data that is not specific to each source region (e.g., MGXS data, etc), as well as the physics methods for updating fluxes between iterations etc. Basically, we are now separating out the storage vs physics/simulation functionality into two different classes, which I think will be a useful abstraction.One might also wonder why there is only one type of
SourceRegion
instead of having flat and linear versions. It seems wasteful to include the extra fields when just doing a flat source simulation, however, when the objects are added into theSourceRegionContainer
, the SoA arrays for the linear terms will not be allocated if doing flat source, so there will be no wasted memory.Motivation
Specifically, this PR is in support of the upcoming "cell-under-voxel" feature for random ray (#2832) that will allow the overlaying of an arbitrary mesh geometry over the simulation so as to allow for automated subdivision of source regions. As the total number of physical source regions will not be known a priori when doing automated on-the-fly source region subdivision, the algorithm becomes highly dynamic.
SourceRegion
objects will be created when new physical source regions are discovered, and then travel through a variety of shared memory data structures so as to minimize locking. Eventually, they will get added to the newSourceRegionContainer
class for permanent residence. Base material filled cell instances will also be created (as a second SourceRegionContainer instance) where external source data will reside.OpenMPMutex Bug Fix
Each source region has a
OpenMPMutex
object that is used to control memory access during transport. Due to the way I initialize the new data structure, the copy constructor ofOpenMPMutex
is now being called when it wasn't before, resulting in the discovery of an error in this class. I corrected the copy constructor so that it is working as expected now, and the underlying openmp locks are being handled correctly. I thought about putting this in a separate PR, but it is good to include here to avoid having to do backflips to make this PR work independent of the mutex bug fix.In the old implementation, the copy constructor was (attempting to) call the regular class constructor, though this was actually just initializing a new object and immediately destroying it, resulting in the new object having a lock that is uninitialized. The new version appropriately initializes the lock in the copy constructor. Notably, it doesn't have to copy anything from the object it is copying, as locks need to be unique and should not actually be copied, so we just make a new one when the copy constructor is being called. The old assignment operator was working, but had an unneeded object creation/destruction, so that has been removed.
Performance Impacts
As the underlying contiguous storage representation is basically unchanged, I have not observed any performance degradation. In my testing, it was actually about 2% faster (though this could just be noise). There is no additional memory usage from the new method. In contrast, the simpler AoSoA storage that I tried would have increased memory usage and had about a 30% performance penalty.
Checklist