Cache PackDescriptors#1239
Conversation
| // surprisingly, this seems to be almost free | ||
| if (num_iter > 0) { | ||
| Kokkos::atomic_increment(&hist(num_iter - N_min)); | ||
| Kokkos::atomic_inc(&hist(num_iter - N_min)); |
There was a problem hiding this comment.
Nothing to do with this PR, just removes a compiler warning.
Yurlungur
left a comment
Yurlungur
left a comment
There was a problem hiding this comment.
I like caching being automatic, but I don't think I love using a string. I feel like that could re-introduce performance hits from using string processing. I assume it's tied to meshblocks and mesh because you're imagining different block lists?
Would there be some way to cache the arguments to the pack instead?
It is only tied to the
Yes, we could make the key be a tuple containing vector of strings, a vector of bools, a vector of |
👍 got it.
I would strongly prefer to avoid using a string for caching. But I'm willing to be overruled. |
Sure, it is easy enough to switch to a different key. |
lroberts36
changed the base branch from
lroberts36/refactor-pack-code
to
develop
It turned out to be cleaner to just put the cache in the |
pgrete
left a comment
pgrete
left a comment
There was a problem hiding this comment.
Looks like a nice cleanup with increased functionality (even though we're not using SparsePacks yet).
I only have some clarifying comments/questions.
| using comm_buf_map_t = | ||
| std::unordered_map<channel_key_t, comm_buf_t, tuple_hash<channel_key_t>>; | ||
| using comm_buf_map_t = std::unordered_map<channel_key_t, comm_buf_t>; |
There was a problem hiding this comment.
Why this change?
And if I didn't miss anything, neither that type nor boundary_comm_map are touched otherwise in this PR, so why is this still working?
| const std::vector<bool> &use_regex, | ||
| const std::vector<MetadataFlag> &flags, | ||
| const std::set<PDOpt> &options) { | ||
| const std::string cache_label{"normal"}; |
There was a problem hiding this comment.
Are these labels, i.e., "normal" and "uid" only used internally?
Is there other code (beyond this PR) that depends on those labels?
Given that I'm not familiar with sparse packs, these question may be trivial/make no sense.
If they do make sense, then maybe one or two lines of documentation might be useful explaining the choice/intent.
There was a problem hiding this comment.
Yeah, they are entirely internal and no downstream user (and most Parthenon developers) should never have to worry about them. It is just a way to allow different methods of creating PackDescriptors to have their own caches.
|
|
||
| namespace parthenon { | ||
|
|
||
| // A hash struct that can be used as a template class in |
There was a problem hiding this comment.
I trust that the updates here work as expected (probably refactored 1:1 from boost, aren't they?)
There was a problem hiding this comment.
I didn't steal them from Boost, but they seem to work correctly. That being said, I haven't checked their performance at all.
There was a problem hiding this comment.
They should give the same results as the code that we previously had in there though.
|
Check that this MR merges |
3 participants
PR Summary
PackDescriptors created usingMesh,MeshBlockData, andMeshData,MeshBlock, andStateDescriptorare automatically cached in theStateDesciptor. Additionally, the interface toMakePackDescriptoris generalized to work in all cases forStateDescriptor,Mesh,MeshBlockData,MeshBlock, andMeshDataarguments. There is also a relatively simple mechanism for adding cacheing to downstreamMakePackDescriptorfunctions that define their own selector for choosing fields to include in aPackDescriptor. This has been tested downstream with Riot.PR Checklist