Fix/result production performance - #1551
Conversation
get_component_output selects the right solver output container with an immediately-invoked lambda whose return type is deduced with plain `auto`. That strips the reference, so every call copy-constructs the entire container just to read one element out of it and then destroys it again. The `auto const&` on the outside lifetime-extends that temporary rather than avoiding it. The only caller, add_appliance_injection, invokes it once per appliance while accumulating injections onto topological nodes. With N appliances in a math group that is N copies of an N-element vector per scenario: O(N^2) work and N heap allocations where O(1) indexing was intended. Give the lambda and the function an explicit `auto const&` return type. The referent is math_output.solver_output[group], owned by the caller, so it outlives the call. Measured on tests/benchmark_cpp (radial grid, 2605 nodes, 1000-scenario symmetric Newton-Raphson batch), MSVC /O2. Step-level figures come from temporary timers around the parts of produce_output, which are not part of this commit: accumulate appliance injections 1.265 s -> 0.0055 s 228x solve_topological_nodes 1.383 s -> 0.107 s 13x The cost is quadratic in appliances per math group, so larger models pay disproportionately more than this grid does. Behaviour is unchanged: the same element is read, only without the surrounding copy. Signed-off-by: Martin Hofmann <martin.hofmann-3@ei.thm.de>
mgovers
left a comment
mgovers
left a comment
There was a problem hiding this comment.
Hi @marhofmann,
Thank you for the great finding and fixes. This indeed is a significant performance regression we should not have.
Regarding the "small improvement", can you please scope it out to a separate PR so that we can fast-track the big fix?
|
Given your extensive benchmark, it would be very useful for us to have some more fine-grained numbers. Would you be willing to have a look at it? |
431e042 to
c763f48
Compare
|
Copy-pasting the details of the thread #1551 (comment) so it doesn't get lost in the resolved comments.
|
|
Thanks, that all makes sense! I've scoped it out and #1551 contains only the |
Follow-up: fine-grained numbersHappy to. The single figure in the PR description understates this, because the cost is quadratic in the number of injections in a math group, not constant. So the scaling is the interesting part. Scaling
Injections = sources + loads + generators, i.e. what the accumulation loop actually walks.
The speed-up grows with grid size because the complexity changes, not the constant factor. It is O(N²) before and O(N) afterDividing by the appropriate power of N gives a near-constant on each side (202-node point excluded; fixed overheads dominate there):
Over a 12.1× range of N: The clearest single viewShare of batch runtime spent producing output:
On At 53 068 nodes, unmodified Where the time sits inside result productionDefault grid (2605 nodes, 1000-scenario batch), with temporary timers around the parts of
Writing every node, branch and appliance record is 0.124 s — 3.5 % of runtime. Everything else in Two rows are controls: building the per-node vectors is untouched by this change and measures 1.000× across it, and Math Calculation is flat. So the gain is localised, not a general shift. What made me look at that loop in the first place: summing injections cost 61.9× more than writing every appliance's complete output record — strictly less work, far more time. How to reproduce the scaling tableNo instrumentation needed — only power_grid_model::Idx constexpr batch_size = 50;
std::array<power_grid_model::Idx, 5> const sizes{1000, 2000, 8000, 24000, 48000};
for (auto const n_spec : sizes) {
power_grid_model::benchmark::PowerGridBenchmark benchmarker{};
power_grid_model::benchmark::Option option{};
option.n_node_total_specified = n_spec;
option.n_mv_feeder = 20;
option.n_node_per_mv_feeder = 10;
option.n_lv_feeder = 10;
option.n_connection_per_lv_feeder = 40;
option.has_measurements = false;
option.has_fault = false;
option.has_tap_changer = false;
option.has_mv_ring = false;
option.has_lv_ring = false;
benchmarker.run_benchmark(option,
{.calculation_type = power_flow,
.calculation_symmetry = symmetric,
.calculation_method = newton_raphson},
batch_size);
}The per-step breakdown needs extra timers, which are not in this PR. Happy to share that patch — Caveats
|
|
Hi @marhofmann, Thanks for finding and fixing this! Would you be open to present your findings in one of the community meetings? We hold community meetings every 8 weeks. |
mgovers
added
bug
|
Hi @marhofmann, Your fix was merged into Regarding your proposal to use |
Thanks! I'd really like to do that, sounds like a nice opportunity. Unfortunately I can't make 2 September. Could I join one of the later meetings instead? |
No worries, I also already resolved the merge conflict with #1551 as well, doing another one will be minor. I expect your PR to be fast-tracked. I do foresee many other places in our code being able to benefit from the small_vector as well. Applying that it's probably a bit more involved, so let's scope that out to yet another PR, but with that potential future improvement in mind, can you please implement the small_vector declaration in the following generic way?
|
Great! Joining a later meeting is possible of course. I'll send you an email to discuss further. |
4 participants
While looking into something else entirely, I got curious about where the time actually goes when a
batch calculation writes out its results. I assumed it would be spread fairly evenly over writing
the node, branch and appliance records. It wasn't. Nearly all of it sat in one loop, and at a rate
that didn't add up: adding a single complex number onto a node was costing something like sixty
times more than computing and writing that same appliance's entire output record.
That seemed odd enough to chase down. It turned out to be two separate things, one big and one
small.
The big one: the whole result vector gets copied for every single value read
get_component_output()returns one element out of one of the solver's result containers. It pickswhich container using a small lambda:
The lambda has no explicit return type, so
autodeduction drops the reference and thatreturncopy-constructs the entire vector. The
auto const&on the outside then binds to the copy andextends its lifetime, which looks like it is avoiding a copy but isn't — the copy has already
happened.
So every call allocates, copies the whole container, reads one element out of the copy, and throws
it away. Its only caller runs once per appliance while totalling injections onto nodes, so with N
appliances each scenario performs N copies of an N-element vector. Quadratic, where plain indexing
was obviously the intent.
The fix is just to write the return type out —
auto const&on the lambda and on the function. Whatcomes back is then a reference into
math_output.solver_output[group], which the caller owns andwhich outlives the call.
Since the waste grew with the square of the appliance count, larger models were quietly paying much
more than the grid I measured on.
The small one: a heap allocation per node, per scenario
With that out of the way, what remained was the step that builds the per-node output vectors. Each
node gets a vector holding one value per user node — but a node only ever holds more than one when
links have merged several nodes into one. In a grid without links, every single one of them holds
exactly one number.
So the code was asking the allocator for room to store one complex number, once per node, for every
scenario. In the benchmark grid that is 2605 allocate-and-free pairs per scenario, roughly 2.6
million across a 1000-scenario batch. The ~37 ns per node I measured is a malloc, not arithmetic.
boost::container::small_vector<ComplexValue<sym>, 1>keeps room for one value inside the objectitself and only goes to the heap beyond that. That's why the inline size is 1: it covers the
ordinary node exactly, and merged nodes still work, they just allocate as before. Boost is already a
required dependency here (
supernodes.hppandtopology.hppuse Boost.Graph) andsmall_vectorisheader-only.
Why I believe this is safe
The second change alters a member's type, so that's the one worth being suspicious of. I tried
fairly hard to prove it wrong before accepting it.
There is exactly one place in the tree that constructs this member. Everywhere else only indexes
into it, and
[i]behaves identically for astd::vectorand asmall_vector. Worth mentioningthat three unrelated things in this codebase are called
bus_injection, and most of the search hitsbelong to
SolverOutput, which this doesn't touch at all.The same function has a second branch for short-circuit output, and that specialisation of the
struct has no
bus_injectionmember in the first place, so there is nothing there to affect.And if I had missed a site somewhere, it would not quietly do the wrong thing:
small_vectorhas noconverting constructor from
std::vector, so anything left over would be a hard compile errorrather than a silent conversion. I checked that directly instead of assuming it. The project builds
with warnings as errors, so there was nowhere for a missed site to hide.
The case I was most worried about is a node holding more than one value, since that is where the
inline capacity spills to the heap. The
handling-of-linksvalidation cases cover exactly that, andthey pass unchanged.
The first change I'd argue is safe more simply: the same element is read either way, just without
the copy wrapped around it.
Numbers
tests/benchmark_cpp, radial grid, 2605 nodes, batch of 1000 scenarios, symmetric Newton-Raphson,MSVC
/O2. Clean builds ofmainand of this branch with no measuring code in either, medians offour runs:
mainThat's 1.63x faster, 38.6 % less total time.
I also ran a separate build with temporary timers inside result production to confirm the saving
really came from these two places rather than from something else moving. Those timers are not part
of this PR. They attribute 1.33 s to these two steps against the 1.40 s measured end to end, and a
neighbouring step that neither change touches stayed flat across both, so I don't think this is just
the machine having a good day.
Testing
All ten C++ test suites pass, 177415 assertions, built with warnings as errors. That includes the
validation data set and the link cases mentioned above.
tests/cpp_unit_tests/main_core/test_topological_node_output.cppcovers this code directly andpasses unchanged.
Happy to split this into two PRs if you'd prefer them reviewed separately — the first commit is a
bug fix and the second is an optimisation, and they're independent.
Note for maintainers
This needs a label to satisfy
check-pr-labels, and I can't add one myself. The first commit is abug, the second animprovement.Assisted-by: Claude (Anthropic)