[Algs] add buf to checksum utility (for debug)#1773
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a timestep callback system, MPI timer tracking, and FNV-1a buffer checksumming, alongside profiling entries and OpenMP parallelization in the SerialPatchTree. However, several critical issues and debug leftovers must be addressed: a continue statement in the benchmark script skips the main loop, a logic error in Solver.cpp checks the wrong callback before execution, and a mandatory environment variable for checksumming could cause startup crashes. Additionally, the inclusion of synchronous file I/O and checksumming in the NeighbourCache will severely degrade performance. The PR also contains poorly named stack entries and an excessive increase in unit test iterations that may be unsuitable for standard CI runs. No feedback was filtered out as all comments identified valid bugs or improvement opportunities.
| #include <fmt/base.h> | ||
| #include <fstream> | ||
|
|
||
| std::string checksum_prefix = shambase::get_check_ref(shamcmdopt::getenv_str("CHECKSUM_PREFIX")); |
There was a problem hiding this comment.
Initializing a global variable using shambase::get_check_ref on an environment variable will cause the application to crash during startup if CHECKSUM_PREFIX is not defined. This makes the environment variable mandatory for all users of the SPH model. Consider using shamcmdopt::getenv_str("CHECKSUM_PREFIX").value_or("") and guarding the subsequent debug file operations with a check for an empty string.
| step_begin=callback_before_mpi_timer, step_end=callback_after_mpi_timer | ||
| ) | ||
|
|
||
| continue |
There was a problem hiding this comment.
The continue statement here will cause the benchmark loop to be skipped entirely. This appears to be leftover debug code that should be removed to allow the benchmark to run.
| tstep.end(); | ||
|
|
||
| for (auto it = timestep_callbacks.rbegin(); it != timestep_callbacks.rend(); ++it) { | ||
| if (it->step_begin_callback) { |
There was a problem hiding this comment.
The condition checks step_begin_callback but the following line attempts to call step_end_callback. This will lead to a crash if step_end_callback is not provided while step_begin_callback is, or it will skip the call if only step_end_callback is provided. It should check it->step_end_callback instead.
| if (it->step_begin_callback) { | |
| if (it->step_end_callback) { |
| = checksum_prefix + "/" + fmt::format("patch_{}_debug.txt", patch_id); | ||
|
|
||
| { | ||
| std::ofstream patch_file(checksum_file_path, std::ios::app); |
There was a problem hiding this comment.
Performing file I/O and buffer checksums (which involve synchronous device-to-host copies via copy_to_stdvec) inside the neighbor cache construction will severely degrade performance. This debug logic should be removed or explicitly guarded by a runtime flag to avoid penalizing production runs.
|
|
||
| Tscal h_tolerance = solver_config.htol_up_coarse_cycle; | ||
|
|
||
| NamedStackEntry stack_loc1ddddd{"wait queue"}; |
There was a problem hiding this comment.
The stack entry names like stack_loc1ddddd and stack_loc1ccccc appear to be temporary debug names and should be renamed to something more descriptive for production code. Additionally, if these entries represent duplicated logic, consider refactoring into a helper function or lambda to improve maintainability.
References
- Refactor duplicated logic into a helper function or lambda to improve readability and maintainability.
| for (u32 i = 0; i < 1000; i++) { | ||
| test.check(); | ||
| } |
There was a problem hiding this comment.
Running the exclusive scan test 1000 times with 32M elements each will significantly increase the duration of the unit test suite. This might be excessive for standard CI runs. Additionally, ensure the scan implementation adheres to the optimization rule regarding accumulator types for non-negative sequences.
References
- When performing a scan (e.g., std::exclusive_scan) on a sequence of non-negative numbers, if the total sum is confirmed to fit within a specific integer type, there is no need to use a wider type for the scan's accumulator, as intermediate sums will not overflow.
Workflow reportworkflow report corresponding to commit cbd3e14 Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Doxygen diff with
|
1 participant
No description provided.