Using swapping in places explicit copying could been avoided #1140
Conversation
Signed-off-by: Jerry Guo <[email protected]>
Signed-off-by: Jerry Guo <[email protected]>
Signed-off-by: Jerry Guo <[email protected]>
Signed-off-by: Jerry Guo <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes performance by replacing explicit copying with swapping operations in two key areas of the codebase to avoid unnecessary data copies.
- Replaced explicit copying with vector swapping in the Y-bus element counting sort algorithm
- Replaced
std::ranges::transformwith structured bindings in dense mapping construction to avoid copying from pairs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| y_bus.hpp | Modified counting sort to use vector swapping instead of copying elements between temporary vectors |
| index_mapping.hpp | Replaced transform operations with structured bindings to avoid pair copying during dense mapping construction |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/y_bus.hpp
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/math_solver/y_bus.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Jerry Guo <[email protected]>
|
| // Use structured bindings to avoid copying from pairs | ||
| for (auto&& [value, orig_idx] : mapping_to_from) { | ||
| result.indvector.push_back(value); | ||
| result.reorder.push_back(orig_idx); | ||
| } |
There was a problem hiding this comment.
I expect this to be overoptimization for Idx due to compiler optimization (obtaining first from std::pair<Idx, Idx> is trivial to optimize).
At the very least, it should be auto, not auto&&, because it's trivial to copy.
| // Use structured bindings to avoid copying from pairs | |
| for (auto&& [value, orig_idx] : mapping_to_from) { | |
| result.indvector.push_back(value); | |
| result.reorder.push_back(orig_idx); | |
| } | |
| // Use structured bindings to avoid copying from pairs | |
| for (auto [value, orig_idx] : mapping_to_from) { | |
| result.indvector.push_back(value); | |
| result.reorder.push_back(orig_idx); | |
| } |
or if you want to use perfect forwarding
| // Use structured bindings to avoid copying from pairs | |
| for (auto&& [value, orig_idx] : mapping_to_from) { | |
| result.indvector.push_back(value); | |
| result.reorder.push_back(orig_idx); | |
| } | |
| // Use structured bindings to avoid copying from pairs | |
| for (auto&& [value, orig_idx] : mapping_to_from) { | |
| result.indvector.push_back(std::forward<decltype(value)>(value)); | |
| result.reorder.push_back(std::forward<decltype(value)>(orig_idx)); | |
| } |
There was a problem hiding this comment.
I agree this one is overoptimization and a bit far fetched. Can be removed.
| for (auto it_element = vec.crbegin(); it_element != vec.crend(); ++it_element) { | ||
| count_vec[--counter[it_element->pos.second]] = *it_element; | ||
| for (auto it_element = vec.rbegin(); it_element != vec.rend(); ++it_element) { | ||
| temp_vec[--counter[it_element->pos.second]] = std::move(*it_element); // NOSONAR |
There was a problem hiding this comment.
why is nosonar needed? multiple statements?
| } | ||
|
|
||
| // counting sort element | ||
| inline void counting_sort_element(std::vector<YBusElementMap>& vec, Idx n_bus) { |
There was a problem hiding this comment.
can you please add a unit test? this stuff is a good optimization, but it's also very easy to make mistakes.
requirements from my end: unit test passes both on main and on this branch. probably best to create a PR directly off main containing only the unit test.
In two places in code, the explicit copying could've been avoided by swapping: