Development for v7.1.0 #351
Merged
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
Member
dschlaep
commented
Jun 9, 2023
•
dschlaep
commented
- Reentrancy and thread-safety #346
The future goal is to rid SOILWAT2 of globals to have the ability to run it concurrently, the following are the first steps toward it New struct SW_ALL in SW_SoilWater.h * Purpose is to contain all structs in SOILWAT2 (SW_VegProd, SW_Weather, etc.) * Currently contains SW_VegProd SW_VegProd has been moved to the new struct SW_ALL and can no longer be accessed as a global * The struct (both SW_ALL and SW_VegProd) is passed to any function in need of the struct/information * Certain *.h files gain an include for SW_VegProd.h and are removed from the respective *.c files due to function headers needing the type SW_VEGPROD Tests gain a global instance of SW_ALL in place of the global for SW_VegProd
- Comprehensive struct, "SW_ALL", gains SW_WEATHER as a new variable - Program loses access to the global instance of SW_WEATHER - The only access to SW_WEATHER is through parameters * Certain functions (e.g., `SW_WTH_finalize_all_weather()`) gain SW_WEATHER access through parameters * Certain functions (e.g., `_end_day()`) lose SW_VEGPROD as a parameter and gain SW_ALL - *.h files in need of SW_Weather.h gain an include for it - Tests have been modified to use the updated SW_ALL global for weather
- Previously, when handling output, this function would be called - However, after rewriting how the output is done in the past, it is no longer in use and is better to be removed
- In recent commits, `SW_OUT_write_today()` gained branches of a switch statement to accommodate for varying parameters in the `get_*_text()` functions - To allow for this to be undone, all `get_*_text()` gain SW_ALL instead of their respective struct - Updated pointer to output routine for text in SW_Output.c/h which now includes a pointer to SW_ALL, to match `get_*_text()` parameters - Mock functions in SW_Output_mock.c gain the same parameter changes * Mock functions do not use parameters, so the use of parameters is faked by uselessly casting them (prevents compiler warnings) - `_echo_outputs()` gain the same fate as mock functions with uselessly casting a parameter - `get_*_SXW()`, `get_*_agg()`, and `get_*_mem()` gain same the same parameters as `get_*_text()`
In the quest for making SOILWAT2 thread-safe, circular dependencies are destined to show themselves, and the only solution is to create a file just for datatypes, hence SW_datastructs.h - New file SW_datastructs.h: * Contains all data structs SOILWAT2 deals with * Respective .h files have lost their instance of structs and are accessed through this new file * Carries three custom types that necessary for one or more given structs within the file * The file itself has sections showing which structs belong to specific sections of the program (Weather, VegProd, VegEstab, etc.) * Gains enums that are required by SW_OUTPUT type SW_Flow_lib.h: - Lost local defines and have been turned into constants in SW_Flow_lib.c * Unused constants have been commented out for the foreseeable future SW_Defines.h: - Gains definitions used by new file SW_datastructs.h
As part of creating the new SW_datastructs.h file, it was realized that not all includes in .c files may be necessary so, unnecessary ones have been removed for less clutter/easier to see what files depend on
* SW_SOILWAT type has been added to SW_ALL * C/header files: - Removed global instance of SW_SOILWAT - Any functions that deal with a struct of type SW_SOILWAT, will now have it passed as a parameter * Unless the function already has access to SW_ALL, in which case no change to parameters is necessary - Modified all instances of accessing the previous "SW_Soilwat" global to match the new parameter Tests: - Lose access to any old (global) instance of SW_SOILWAT - All tests in need of it, access SW_SOILWAT through the global instance of SW_ALL - sw_testhelpers.h now directly accesses new SW_datastructs.h file
- Previous, if a function used an attribute of a struct once, it gained the struct as a parameter - To remove possible confusion, a loose convention, if at least three different attributes are used, the function gains the struct * If this condition is not met, functions gain the individual necessary information - Tests conform their function calls to the changes in parameters
- SW_ALL gains SW_MODEL C/Header files: - Removed all global instances of SW_MODEL - All functions that use data from this struct, will either have the struct itself or have the individual variables passed in * This is to keep the convention mentioned in commit 942ec7d going Tests: - Tests have been updated to match modified parameters - Anything in need of information from SW_MODEL will now have to access it through "SW_All"
- SW_ALL now contains SW_MODEL C/header files: - Removed all global instances of SW_SITE - All functions that use data from this struct, will either have the struct itself or have the individual variables passed in * This is to keep the convention mentioned in commit 942ec7d going Tests: - Tests have been updated to match modified parameters - Anything in need of information from SW_SITE will now have to access it through "SW_All" Defines: - Macros dealing with looping through layers must have the upper-bound sent in - "ForEach...TranspLayer" defines have been simplified * Compressed down to one, so it must have vegetation index and upper-bound sent in * Only defined when STEPWAT2 is as well
- SW_ALL now contains SW_VEGESTAB C/header files: - Removed all global instances of SW_VEGESTAB - All functions that use data from this struct, will either have the struct itself or have the individual variables passed in * This is to keep the convention mentioned in commit 942ec7d going * `_new_species()` in SW_VegEstab.c does not follow this due to complications updating values properly - Tests gain usual changes to conform to updates of parameters
-> argument order of `SW_VES_read2() now agrees between implementation and header -- and is as used by `SW_VES_read()` -> fix call to `SW_VES_read2()` by unit test
Source/include files: - Focusing on commits starting at eab14cf - Fixed missing documentation of function parameters * If documentation wasn't missed, added/fixed direction specifiers (i.e., [in], [out], and [in,out]) - Functions using more than two structs from the comprehensive structure gain the comprehensive struct to follow the convention mentioned in 942ec7d - Modified some functions interface to match the order of parameter documentation Tests - Any functions that had their interface modified that are called in tests have been adjusted in the test files
- SW_ALL gains SW_SKY C/Header files: - Removed all global instances of SW_SKY - All functions that use data from this struct, will either have the struct itself or have the individual variables passed in * This convention is described in commit 942ec7d * This only applies to `SW_SKY_read()` in this case Tests: - Tests have been updated to match modified parameters (only `SW_WTH_read()`) - Anything in need of information from SW_SKY will now have to access it through "SW_All"
- SW_ALL gains SW_CARBON C/Header files: - Removed all global instances of SW_CARBON - All functions that use data from this struct, will either have the struct itself or have the individual variables passed in * This is to keep the convention mentioned in commit 942ec7d going - Updated function documentation to include new parameters Tests: - Tests have been updated to match modified parameters - Anything in need of information from SW_CARBON will now have to access it through "SW_All"
- existing argument "avgLyrTemp" now provides as input yesterday's soil temperature values (new behavior, replacing behavior of "oldavgLyrTemp") and obtains today's soil temperature values as output (continuing behavior)
- directly use `SW_SoilWat->swcBulk[Today][]` instead of `lyrSWCBulk[]` - directly use `SW_SoilWat->drain[]` instead of `lyrDrain[]` - move variable `drainout` inside `SW_Water_Flow()`
- use `SW_SoilWat->avgLyrTemp[]` directly instead of `lyroldavgLyrTemp[]` and `lyravgLyrTemp[]`
…urfaceAvg` - the "SW_Flow" module had a static two-day array `surfaceAvg` to store yesterday's and today's soil surface temperature - however, yesterday's values have never been used -> eliminate module-level array and elminate copying rom arrays to `SW_Weather->surfaceAvg` (this already contains the values since it is now used directly) -> simplify interfaces of `soil_temperature()` and `SW_ST_setup_run()`
-> "SW_Model" is now "sw->Model"
…ow module - use `SW_SoilWat->hydred[][]` directly instead of `hydred[][]`
- use `SW_SoilWat->transpiration[][]` directly instead of `lyrTransp[][]`
- clarify names of elements that hold bare-soil evaporation (and not total evaporation) - struct `SW_SOILWAT`: "evaporation" renamed to "evap_baresoil" - struct `SW_SOILWAT_OUTPUTS`: "evap" renamed to "evap_baresoil"
- argument "qty[]" of `remove_from_soil()` is now updated, i.e., 'input of `qty`' + 'delta moisture', instead of just returning 'delta moisture' - use `SW_SoilWat->evap_baresoil[]` directly instead of `lyrEvap[][]` and `lyrEvap_BareGround[]` -> `arrays2records()` is no longer needed
- the interface of the debugging function `SW_WaterBalance_Checks()` was updated from many structs to `SW_All *` by commit f1a2a04 "Catch-up on function documentation" - however, the update was not propagated to the header -> this is now fixed
- Mistakenly, the previous commit, 32e76a3, did not work in STEPWAT2 * Initialization did not occur fast enough in the program for STEPWAT2 to set values to zero * The idea in the mentioned commit was correct, but not properly executed - Moved Initialization to the beginning of `SW_Water_Flow()` after `SW_ST_setup_run()`
* Initialize all soil temperature during `SW_ST_setup_run()` - initial soil temperature values are obtained from "soils.in" and stored in `SW_Site.avgLyrTempInit[]` (renamed from previous `SW_Site.avgLyrTemp[]`) - previously, soil temperature values were set to initial values at two different locations ** `SW_SoilWater.avgLyrTemp[]` (soil temperature at depths across the soil layer profile) were copied from `SW_Site.avgLyrTempInit[]` with call to `SW_SWC_read()` -- however, this occurred whether or not the soil temperature module was activated ** `SW_StRegValues.oldavgLyrTempR[]` (soil temperature at depths across the soil temperature profile) were afterwards interpolated from `SW_SoilWater.avgLyrTemp[]` with call to `SW_ST_setup_run()` - previously, this did not work correctly if the soil temperature module was turned off because soil temperature output reflected constant values from `SW_Site.avgLyrTempInit[]` (via `SW_SoilWater.avgLyrTemp[]`) - earlier attempts to solve this problem included ** commits 32e76a3 "Initialize average layer temperature to zero in `soil_temperature()`" didn't work correctly ** commit 0d0a162 "Fix initialization of avg. layer temp. in `soil_temperature()`" added an expensive daily (instead of initial) operation (that was also superfluous if the soil temperature module is activated) -> this commit re-organizes initialization of soil temperature values - now, `SW_ST_setup_run()` sets all soil temperature to initial values (which is only called if the soil temperature module is activated), i.e., both `SW_SoilWater.avgLyrTemp[]` and `SW_StRegValues.oldavgLyrTempR[]` are derived from `SW_Site.avgLyrTempInit[]` * Improve documentation related to soil temperature - fix spelling - fix and update input/output usage of parameters - provide additional explanation
- First commit: aee615d - Remove missed movement of LOG_INFO parameter to last except `LogError()`, the mentioned commit provides more insight
- `LogError()` is provided with a string of a format that does not allow any variables - Removed variable being sent in
- Support a convention mentioned by googletests https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-naming * Summary: if a test suite contains a death test, name it "*DeathTest" - When a test suite contains a death test, the suite name is - AllTest --> AllDeathTest
- p_OUT is no longer global on SOILWAT2
- close #356 - given that `SW_SoilWat->hist.file_prefix` is not initialized when `SW_SWC_construct()` is called (by SOILWAT2), then `!isnull(SW_SoilWat->hist.file_prefix)` was TRUE and we freed the not allocated pointer. -> now, initialize instead of free the pointer -> NOTE: rSOILWAT2 may now have a memory leak if it calls this function with a previously allocated `file_prefix` (e.g., if a `sw_exec()` fails and doesn't properly clean up).
- close #205 - remove leak suppression in "SW_VPD_construct" that is no longer needed - the test "VegProdStructTest.VegProdConstructor" (previously named, as of v7.0.0, "VegTest.Constructor") now (1) uses a local copy of SW_VEGPROD and (2) calls `SW_VPD_deconstruct()` to avoid a memory leak - Previously, the test used `SW_All.VegProd` or a global variable (for which `SW_VPD_construct()` has already been called once and allocated memory, e.g., during the test fixture's `SetUp()` or `Reset_SOILWAT2_after_UnitTest()`); then the (second) call to `SW_VPD_construct()` would allocate memory a second time (thus the memory previously allocated is leaked). However, the call to `SW_VPD_deconstruct()` (either in the test or during the test fixture's `TearDown()` or `Reset_SOILWAT2_after_UnitTest()`) will de-allocate only the most recently allocated memory (and, if called again, was seeing only NULL). - before this commit, I was getting with `MallocStackLogging=1 MallocStackLoggingNoCompact=1 MallocScribble=1 MallocPreScribble=1 MallocCheckHeapStart=0 MallocCheckHeapEach=0 leaks -quiet -atExit -- bin/sw_test` > 7 leaks for 896 total leaked bytes. 7 (896 bytes) << TOTAL >> 1 (128 bytes) ROOT LEAK: 0x600000cb0100 [128] 1 (128 bytes) ROOT LEAK: 0x600000cb0180 [128] 1 (128 bytes) ROOT LEAK: 0x600000cb0200 [128] 1 (128 bytes) ROOT LEAK: 0x600000cb0280 [128] 1 (128 bytes) ROOT LEAK: 0x600000cb0300 [128] 1 (128 bytes) ROOT LEAK: 0x600000cb0380 [128] 1 (128 bytes) ROOT LEAK: 0x600000cb0400 [128] - after this commit, I'm getting > 0 leaks for 0 total leaked bytes.
- `*PathInfo` was only used to print error messages; however, it required three other functions that call `SW_SIT_init_run()` to provide `*PathInfo` just for that purpose -> eliminate argument `PATH_INFO *PathInfo` and simplify error messages -> `SW_CTL_init_run()`, `set_soillayers()`, `create_test_soillayers()` also lose the no longer needed argument `PATH_INFO *PathInfo`
- run "leaks" command on example simulation and on test executable
Squashed commits from branch feature_reorganizeTests. - test fixtures * use synonyms of test class fixture "AllTestFixture" to group tests in suitable groups (suites) * only use test fixtures where really needed * do not use test fixtures in death tests (see below) - test class for death tests * new class "AllTestStruct" that behaves just like "AllTestFixture" but does not inherit from `::testing::Test` * only use class "AllTestStruct" where really needed * do use class "AllTestStruct" inside `EXPECT_DEATH` assertion - death tests * should avoid the creation (SetUp or constructor) of multiple instances of "AllTestStruct" or "AllTestFixture" * each `EXPECT_DEATH` "re-executes the binary to cause just the single death test under consideration to be run" in thread-safe mode (https://google.github.io/googletest/reference/assertions.html#death). * thus, all code inside the death test and before the `EXPECT_DEATH` assertion is run twice (including setting up of a test fixture). * therefore, place relevant code inside `EXPECT_DEATH` in a compound statement * use a local copy of "AllTestStruct" that is created inside a compound statement inside `EXPECT_DEATH` assertions if death tests requires SOILWAT2 example data - names * follow GoogleTest guidelines (https://google.github.io/googletest), e.g., no underscores; test fixtures and test suites end in *Test; death test suites end in *DeathTest * name of test fixtures ends in *FixtureTest * name of test suites that do not use the test fixtures have a name matching the name of the test fixture - but without the "Struct" part in the name * name of test suites that do use the class "AllTestStruct" have a matching name that contains "Struct" (e.g., instead of "Fixture") * for instance, tests for "Site" functionality are named as follows ** name with a test fixture: "TEST_F(SiteFixtureTest, Site*)" ** name of death test that uses "AllTestStruct": "TEST(SiteStructDeathTest, Site*)" ** name of death test that uses neither "AllTestStruct" nor "AllTestFixture": "TEST(SiteDeathTest, Site*)" ** name of test suite that uses neither "AllTestStruct" nor "AllTestFixture": "TEST(SiteTest, Site*)" - new test helper functions - new `silent_tests()` to simplify local use of `LOG_INFO` in tests - new `setup_SW_Site_for_tests()` to simplify local use of `SW_SITE` in tests - new `setup_AllTest_for_tests()` to set up/construct both "AllTestFixture" and "AllTestStruct"
Feature memory fixes - fix memory issues (close #205, close #356) - `SW_SIT_init_run()` does not need argument `PATH_INFO *PathInfo` - Reorganized tests - only use test fixtures where really needed - do not use test fixtures in death tests - new class "AllTestStruct" that behaves just like "AllTestFixture" but does not inherit from `::testing::Test` -- which can be used in death tests - place all relevant code inside `EXPECT_DEATH` in a compound statement -- code will otherwise be run twice (including the creation of test fixtures or constructing of a local copy of "AllTestStruct") - tool "check_SOILWAT2.sh" now runs additional steps with the `leaks` command
- Previously, the macro was given a parameter, n_layers, that was not used - Remove this parameter and update calls
…safety * Prepare for SOILWAT2 to become thread-safe and reentrant (close #346) * Definition clarifications * Thread-safe - Multiple threads (a future SOILWAT2 development) will not influence each other unintentionally. Here, we implemented structures to enable thread-local storage, i.e., each thread operates on local data structures or with static data. * Reentrant - The ability to correctly execute any part of the program by multiple threads simultaneously but independently from others. * All non-static variables are replaced by local variables; functions gained arguments to pass local variables by reference (replacing global variables). * New main abstract types * SW_ALL - Contains the existing structures that are relevant for the simulation itself, e.g., SW_MODEL, SW_SOILWAT, SW_WEATHER. * PATH_INFO - Holds information about location of input and output data, e.g., directories, file paths to input data. * SW_OUTPUT_POINTERS - Points to requested output subroutines. * LOG_INFO - Manages information for logging warnings and errors.
- close #339 - https://github.com/google/googletest/releases/tag/v1.14.0 - see also commit a490201 which upgraded our tests to compile with c++14
- Some functions only use/gain an argument of "InFiles" for error messages * Error messages use hardcoded file names * Removed "InFile" as a parameter from these functions and updated calls to said functions
N1ckP3rsl3y
approved these changes
Aug 4, 2023
dschlaep
added a commit
to DrylandEcology/rSOILWAT2
that referenced
this pull request
Aug 4, 2023
Accommodate SOILWAT2 v7.1.0 developments (DrylandEcology/SOILWAT2#351) * This version produces the same output as the previous version. * Update `SOILWAT2` to v7.1.0 which prepares for thread-safety and reentrancy * C-side of `rSOILWAT2` gains new globals of the four main abstract types in `SOILWAT2` - SW_ALL, SW_OUTPUT_POINTERS, LOG_INFO, and PATH_INFO - to interact with `SOILWAT2`.
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.