Optimize setting BMI input variables prior to each time step update.#942
Optimize setting BMI input variables prior to each time step update.#942robertbartel wants to merge 27 commits into
Conversation
|
Caching this seems like the right move! I'm concerned with the amount of duplicated data in this design but I'm likely missing how things fit together. Can you check my understanding? Let's say we had a multi-bmi w/ 3 inner formulations applied uniformly over 20 divides. So, we would have 3*20 formulation instances + 20 multi-bmi wrapper formulations. Each formulation would contain a |
Pretty sure this is right. I did give some thought to the memory inefficiency of this. Basically, I believe a more efficient design is easily possible but would require much more time and effort in the design and testing. Conversely, the effects of this on memory are relatively small and, I think, acceptable even at scale. And the performance gains are worth the trade. But @aaraney, sanity checking me won't hurt. My ballpark rough estimate is that, for each formulation instance, it would maybe take < 400 bytes of memory to save those details, including stuff in the struct, the vector object itself, etc. I think the strings are the biggest part of that: let's say those are < 100 bytes each, and there are 3. The other stuff I think could be handled in < 100 bytes. Keeping the math easy, let's say CONUS has < 1,000,000 catchments. Assuming a single global formulation entry in the config, and that it has 5 nested BMI module formulations, that's < 2GB extra memory for the entire CONUS simulation. That's certainly not nothing, but I think it's totally safe and acceptable on a system running a CONUS simulation. |
|
I think I forgot to also account for multiple variables per module formulation properly, so this does increase the memory load further by probably around a factor of 5. So, 10GB for a CONUS simulation. Growing larger definitely, and I'm not quite as comfortable, but in my opinion still acceptable given it'll be spread across a lot of processes. |
|
The changes as-is make the assumption that the relevant BMI variable details will not change over course of the simulation. BMI doesn't actually guarantee this. It is likely true for the vast majority of use cases, but could (in theory) break others. To account for this, I'm going to change things to make it a configurable option, at the formulation level, whether to execute using this new caching mechanism or re-query every time (with the latter being the default for safety reasons). This will also give the user some control if, in their use case, they do not want to make the memory-compute tradeoff that originally this change would have imposed. |
Thanks for the ballpark numbers! That's super helpful! My main concern is polluting the higher level cpu caches which reduces performance in other parts of It seems like in the majority of cases the For 1 multi-bmi we've got: 5 formulations seems a little high unless we are running an ensemble in a single run. 3 seems a little more realistic 1206310248=17.69 MiB or a little of half of the L3. That's assuming everything is contiguous and without padding. |
Honestly I wouldn't worry about this until we have the use case. I appreciate the thorough thought! |
Nels and I chatted about this some. The reason we agreed it should be done now is that, if we (or others) ever do encounter a problematic use case, then it's quite possible things will break but appear to function correctly. E.g., the number of items changes for some variable after n time steps, we pass a |
|
I think all the bones are here to get us what we want. I think we can minimally tweak the design and avoid the majority of the duplicate data problem we were talking about above. The main concern is duplicate With that in mind, what if we introduce a global vector of The global vector shouldn't ever be more than ~50 items and even if its is, its still really small in comparison. A similar pattern is already used by the std::vector<Bmi_Var_Metadata> bmi_vars_metadata;
struct Bmi_Var_Metadata {
std::string name;
std::string mapped_alias;
std::string cpp_type;
uint32_t num_items;
uint32_t item_size;
bool operator==(const Bmi_Var_Metadata& rhs) const
{
return (
this->name == rhs.name &&
this->mapped_alias == rhs.mapped_alias &&
this->cpp_type == rhs.cpp_type &&
this-> num_items == rhs.num_items &&
this->item_size == rhs.item_size
);
}
};The slight modification to struct Bmi_Var_Details {
data_access::GenericDataProvider &provider;
const uint32_t &var_metadata_handle;
};The great thing is you already have the codepaths to accommodate this change. So it shouldn't be a huge lift. The main things that would change are:
Modifying the napkin math we did previously now we've got: 12(sizeof( So, if we've got 1024 divides in a rank we would have 360*1024 + 3120 = ~372K. Not bad. OC there is more we could do. But, this seems in the spirit of what you were thinking and isn't a huge deviation. Does this seem reasonable, @robertbartel? |
566b3d8 to
06da486
Compare
|
Refactored and added config support to optionally use these optimization, with the default being to re-fetch things every time as had been done previously. |
I'd like to avoid getting too much further into the weeds on things in this PR. Part of the motivation for it originally was that it was a relatively simple change with clear benefits. There was a memory tradeoff, yes, but one that at a glance appeared acceptable. It turned out that it also wasn't guaranteed to be correct for all BMI modules, so a little more work to make it optional was necessary. That had the additional benefit of protecting against the possibility of any unanticipated negative effects on performance that could emerge in different situations. Any more, though, and I worry this stops being the low-hanging fruit that I was originally targeting. @aaraney do you feel strongly that the current memory implications compel us to not adopt this in it's current form? Otherwise, we can always build more on this in the future. |
|
With NOAA-OWP/nwm-qa#380, it looks like there is a more concrete reason why compute optimizations are desirable right now. So there is now good reason for getting into the weeds if we need to in order to make this work. I'll start working on @aaraney's suggestions. |
|
Regarding enablement being configurable, I think all of our current BMI models would allow caching their input expectations. |
0a2da19 to
4dfe6d4
Compare
61d99cf to
7fbf8cc
Compare
|
Making note of this, but not sure we need to do something about it here. We still seem to be have some sporadic issues with the test_bmi_c check on macOS. While the last retry was good, before that it was failing and giving I've never been able to reproduce something like this locally on my Mac. And, after a few retries (four this time) and without any further changes to the PR branch, the error doesn't happen and the check passes (FWIW, this last retry, I turned on the extra debugging ... unclear if that made any difference, and I don't remember if I did that in any previous clearing retries in #922 that I discuss below). I saw similar |
Adjust function signature to change param name for model time to avoid confusion regarding what might be meant by "init" or "initial" in BMI contexts, and fix docstring for param, which was wrong even before the name change.
Adjusting things further in set_model_inputs_prior_to_update improvements, including - support both executing with these new optimizations or without them (i.e., still fetching and re-calculating input var metadata on each call) - add member variable (default: `false`) and setter function to store whether to execute using new set_model_inputs_prior_to_update optimizations or original "always re-fetch" logic - modularize parts of functionality into several different functions for better organization and reusability - add variable units to metadata that is stored
Adding config option for available set_model_inputs_prior_to_update optimizations that store metadata, along with documentation on use.
Making several adjustments to the optimizations for storing BMI input variable metadata in a formulation for reuse: - make Bmi_Var_Details (effectively immutable) class rather than struct - pull provider pointer out of Bmi_Var_Details and setup a second vector in the formulation object for providers that corresponds to the one for Bmi_Var_Details (providers can be formulation-specific, so relationship belongs there) - created static std::set of Bmi_Var_Details objects that gets added to during logic to populate instance's vector of input Bmi_Var_Details, with the latter vector only having values inserted that exist in this static set - move logic for initializing the metadata-containing collections to a dedicated function
Updating logic in Bmi_Module_Formulation::get_provider_for_input_var to avoid searching a map twice back-to-back for the same key.
Updating with more concise params list that rolls several things into a Bmi_Var_Details object, and then updating usage to match.
Rename store_input_variable_metadata to cache_input_variable_metadata to clarify things are saved in mem not stored on disk; updating macro name and documentation to also reflect and better indicate this.
Renaming related params, functions, and similarly (but not identically) named variables in the same way.
Simplify copy constructor and improve formatting of < overload.
Fixed new tests associated with the optimization changes, and moved the boilerplate present (now, after some rebasing) in prior tests but missing from new ones (related to those tests now breaking) into a reusable function.
Use by-val and moves rather than const ref to reduce copies, per suggestion from @hellite500. Co-authored-by: Nels <nels.frazier@noaa.gov>
Add test that caching input variable metadata yields responses consistent with default execution, where metadata is re-fetched each time.
More tests to make sure Formulation_Manager behaves properly in various situations and gets formulations with appropriate cache flag setting for config.
f2b0a4b to
407b7c7
Compare
|
Update: the errors I was seeing appear to have actually only been due to running out of disk space on my test machine ( 🤦♂️ ), so taking back out of Draft status. |
|
FWIW, retesting a bit today over a handful of runs, I'm seeing between 45 and 50 seconds reduced runtime with these optimizations active, on a 720-time-step simulation that posts |
4 participants
Optimizing
Bmi_Module_Formulation.set_model_inputs_prior_to_updateby storing certain reused details rather than re-calculating or re-retrieving on each time step.Additions
Bmi_Var_Detailsstruct type to hold some reusable information on BMI variablesBmi_Module_Formulation.bmi_input_var_detailswith info on module BMI input variables, stored in aforementioned struct instancesChanges
Bmi_Module_Formulation.set_model_inputs_prior_to_updateto lazily init the contents ofBmi_Module_Formulation.bmi_input_var_detailsand then use that to provide info necessary prior to BMI setting operations, rather than re-querying/re-processing that info on every time step.Bmi_Module_Formulation.set_model_inputs_prior_to_updateTesting
Formulation_Manager_Test.basic_run_3does cover usage of this function. I made temporary local modifications introducing incorrect logic in a few different places inset_model_inputs_prior_to_update, and this did induce failures of that test.Notes
NGen::simulationtime by ~100 seconds.Checklist
Testing checklist (automated report can be put here)
Target Environment support