Give per-user-node vectors inline storage - #1555
Conversation
Each topological node holds one value per user node, and only link-merged supernodes have more than one -- in a grid without links every topological node has exactly one. Three of these vectors are rebuilt on every scenario (node_injection, node_flow_from_branch and bus_injection), so each node asks the allocator for room to store a single complex number three times per scenario. Give them inline room for one element, so the ordinary node does not allocate. Link-merged nodes still work, they just allocate as before. The indexing interface is unchanged. As a side effect the copy from node_injection into bus_injection stops being a heap copy, because both sides are now the same inline type. Expose the container generically rather than at the point of use: a new common/small_vector.hpp holds a single using declaration for SmallVector, alongside the other type aliases in common/. Other call sites can then reach for it without pulling in boost directly, and the underlying container can be swapped in one place. boost::container::small_vector is header-only and Boost::headers is already an INTERFACE dependency of the power_grid_model target, so this adds nothing to the build. The inline capacity stays a per-use-site decision; here it is 1 because that is exactly what an ordinary topological node holds. Measured on tests/benchmark_cpp (radial grid, 2605 nodes, 1000-scenario symmetric Newton-Raphson batch), MSVC /O2, against the parent commit, 10 interleaved rounds per arm on an otherwise idle machine, medians: produce output 2.4487 s -> 2.2470 s 1.09x math calculation 2.0757 s -> 2.1361 s control, unchanged within noise whole batch run 4.9217 s -> 4.7514 s 1.04x The change won 9 of the 10 paired rounds, so the effect is real, but the 0.20 s median gain sits inside main's own 0.69 s run-to-run spread, so treat it as modest rather than decisive. The gain is smaller than it would have been before PowerGridModel#1536, because most of the per-node allocation cost now sits in compute_link_solver, which this does not touch. See the linked issue. All ten C++ test suites pass, 176999 assertions, built with warnings as errors, including the handling-of-links cases that exercise supernodes holding more than one user node. Signed-off-by: Martin Hofmann <martin.hofmann-3@ei.thm.de>
|
Linked issue #1554 |
mgovers
left a comment
mgovers
left a comment
There was a problem hiding this comment.
Hi @marhofmann,
Thanks you once again for your contribution. Everything looks good 👍
Apologies for the change in #1536 that has reduced the benefits of this branch. My educated guess is that that's just repetition of the same/a similar performance regression, in which case applying small vectors there would solve it. I will dive into #1554 now to investigate, but since this PR looks good, let's merge it. It might come in useful for the investigation. Would it be alright if you do another benchmark analysis after our investigation?
Signed-off-by: Martin Hofmann <martin.hofmann-3@ei.thm.de>
Head branch was pushed to by a user without write access
|
There was a format issue. I fixed it |
|
@TonyXiang8787 the bug report #1554 is separate from this and was a new finding during implementation of this ticket. I am working on a fix for #1554 based off this branch. |
eaddbec
|
@mgovers
So on top of #1556 this is worth 1.69×, and together they get ~98.5% of the regression back. Whole run 3.93 s → 2.19 s. |
|
Perfect news and once again thank you for the extensive analysis! |
|
I wonder how many other locations can benefit significally from the small vector. Topology reduction is a very likely candidate. |
3 participants
Each topological node holds one value per user node, and only link-merged supernodes have more than one: in a grid without links every topological node has exactly one. Three of these vectors are rebuilt on every scenario (node_injection, node_flow_from_branch and bus_injection), so each node asks the allocator for room to store a single complex number three times per scenario.
Give them inline room for one element, so the ordinary node does not allocate. Link-merged nodes still work, they just allocate as before. The indexing interface is unchanged. As a side effect the copy from node_injection into bus_injection stops being a heap copy, because both sides are now the same inline type.
Expose the container generically rather than at the point of use: a new common/small_vector.hpp holds a single using declaration for SmallVector, alongside the other type aliases in common/. Other call sites can then reach for it without pulling in boost directly, and the underlying container can be swapped in one place. boost::container::small_vector is header-only and Boost::headers is already an INTERFACE dependency of the power_grid_model target, so this adds nothing to the build. The inline capacity stays a per-use-site decision; here it is 1 because that is exactly what an ordinary topological node holds.
Measured on tests/benchmark_cpp (radial grid, 2605 nodes, 1000-scenario symmetric Newton-Raphson batch), MSVC /O2, against the parent commit, 10 interleaved rounds per arm on an otherwise idle machine, medians:
produce output 2.4487 s -> 2.2470 s 1.09x
math calculation 2.0757 s -> 2.1361 s control, unchanged within noise
whole batch run 4.9217 s -> 4.7514 s 1.04x
The change won 9 of the 10 paired rounds, so the effect is real, but the 0.20 s median gain sits inside main's own 0.69 s run-to-run spread, so treat it as modest rather than decisive.
The gain is smaller than it would have been before #1536, because most of the per-node allocation cost now sits in compute_link_solver, which this does not touch. See the linked issue.
All ten C++ test suites pass, 176999 assertions, built with warnings as errors, including the handling-of-links cases that exercise supernodes holding more than one user node.