Cloudy-table texture refactor (Cool Refactor 2/3)#510
Open
mabruzzo wants to merge 14 commits into
Open
Conversation
Notably, the timing test hasn't actually been affected by a noticeable amount
…accepts ParameterMap
a8ab83c to
524b939
Compare
hannahjleary
approved these changes
Apr 29, 2026
Co-authored-by: Hannah Leary <97999006+hannahjleary@users.noreply.github.com>
This file contains hidden or 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.
To be reviewed after #509 is merged. This is the second in a sequence of 3 PRs to refactor and standardize cooling recipes and components.
Overview
This refactors the cloudy-table texture logic.
The headline change is the creation of the
cool_component::CloudyHeatAndCoolclass. In slightly more detail:CoolRecipeCloudyCoolRecipeCloudyandCoolRecipeCloudyAndPhotoHeatinghave now both been refactored to wrap this type (with this change, a cooling recipe NEVER wraps another cooling recipe)Other Changes
Start using
SharedHandle<cudaTextureObject_t>As part of this PR, I started to make use of
SharedHandle<cudaTextureObject_t>inside ofcool_component::CloudyHeatAndCool. For context, I introduced theSharedHandle<HandleT>class template within #509. Rather than track 2 rawcudaTextureObject_tinstances within the class (for the cooling & heating tables), we now track 2SharedHandle<cudaTextureObject_t>instances.SharedHandle<cudaTextureObject_t>instance wrapscudaTextureObject_thandle and uses it to exercise ownership over the associated texture.get()method.SharedHandle<cudaTextureObject_t>models shared ownershipSharedHandle<cudaTextureObject_t>(e.g. by copying acool_component::CloudyHeatAndCoolthat tracked that instance) the new copy and the original instance share ownership of the associated texture.SharedHandle<cudaTextureObject_t>directly to a GPU kernelAll of this allowed us to stop tracking the
cudaTextureObject_tas global variables and to actually avoid memory leaks (previously we never actually free-ed the memory). The only way to avoid the memory leaks (something we want to do in tests) in a composable manner (i.e. where we can directly pass thecool_component::CloudyHeatAndCoolto a global function) and without move-semantics (more about that in the next paragraph) is if we did some kind of crude reference counting. If we are going to reference count, we might as well use aSharedHandle<cudaTextureObject_t>. (As an added bonus, we can avoid making new global variables for each kind of texture we may want to use).I spent a bunch of time going down a dead-end where I tried giving
cool_component::CloudyHeatAndCoolthe move-semantics of astd::unique_ptr, but the following 2 factors convinced me not to do itCoolingUpdateExecutor<CoolingRecipe>in astd::function<void(Grid3D &)>. This is surmountable: we could either use something like C++23'sstd::move_only_function<void(Grid3D &)>OR we could create a custom abstract base class and storeCoolingUpdateExecutor<CoolingRecipe>in a subclasscool_component::CloudyHeatAndCooldirectly to a global function becomes tricky. We either:cool_component::CloudyHeatAndCoolto a global function act as an exception of move-semantics. While inelegant, I'm not ideologically opposed. I just don't think its a good idea since I'm already concerned about people being intimidated by move-semantics.Other stuff
I also got the the legacy test-logic from
load_cloudy_texture.cu, theTest_Cloudy_Textures()andTest_Cloudy_Speed()functions, running in the automated test suite. I disabled the former (to avoid printing lots of lines).NOTE: they don't actually check the computed values.
Footnotes
A
SharedHandle<cudaTextureObject_t>instance releases ownership over a texture if it the instance goes out of scope (i.e. the destructor gets called), calling thereset()method, or it takes on ownership of a different texture. The last case occurs if you have 2 shared handles (sh_A&sh_B) that don't own the same handle and you perform some kind of assignment (e.g.sh_A = sh_B;) or swap the contents (e.g.sh_A.swap(sh_B)); ↩The precise deletion logic is specified by a callback that is provided when you construct a shared handle ↩