Optimize CSV monitor output for 2x runtime

[lukelowry]

Description

This stacked PR optimizes CSV monitor output in PhasorDynamics by formatting CSV values directly into a reusable line buffer in the hot monitor path. It is stacked on #411 (lukel/perf-sys-dev).

Proposed changes

• Add protected CSV append hooks to VariableMonitorBase with existing stream-print fallbacks.
• Add direct CSV append support for scalar monitor values using std::to_chars for floating-point output.
• Reuse a single CSV line buffer in VariableMonitorController and write each CSV row once.
• Leave JSON/YAML output and direct CSV stream-print methods on their existing stream behavior.

Performance was measured with application/PhasorDynamics/PDSim using the app-reported Complete in time. Each row reports the median of 3 trials after rebuilding PDSim on the corresponding branch.

Case	lukel/perf-sys-dev (s)	This branch (s)	Improvement
Hawaii	0.172773	0.124430	28.0%
NewEngland	0.229073	0.169727	25.9%
Illinois	0.652370	0.319652	51.0%
WECC	1.153270	0.576705	50.0%
Texas	8.437620	4.311520	48.9%

Checklist

• All tests pass. (except the one PE failure)
• Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
• The new code follows GridKit(TM) style guidelines.
• N/A: no new unit tests for this narrow CSV output fast path.
• N/A: no user-facing documentation change.
• The feature branch is based on the target branch.
• N/A: no CHANGELOG.md update is needed for this internal PhasorDynamics monitor-output optimization.

Further comments

stoked

[PhilipFackler]

This approach is great! Nice results. Thanks for spending time on this. Looks like I trusted the std::ostream buffer too much. 😸

I would like you to revert the structure a bit, though.

I think you should use this approach for all of the formats. No need to have extra custom CSV functions and separate bits in the ValuePrinter. Use the string buffer in the controller (call it something generic like buffer_ instead of csv_line_) and change the print* functions internally to use the interface you've made here for appendCsv. You could change them to append also, to make it clearer (don't need two different implementations for the same thing). You shouldn't have to bother much with the implementations for the other formats (just switch to using stringstream).

In other words, just replace my implementation with yours everywhere. 😆

How does that sit with you?

[PhilipFackler]

Also, shouldn't this be aimed at develop?

[lukelowry]

Also, shouldn't this be aimed at develop?

I can change the target branch, I only kept it like this because I measured it against my other branch, not this one! I will shift target so it's async with the other PR.

Good on unifying across the three formats, I just don't know much about yaml. Otherwise as long as you and @pelesh are okay with this, I can certainly extend this to the other formats and make api clearer. Thank you for the feedback!

[PhilipFackler]

I just don't know much about yaml

They all use std::ostream, so you just need to change it to append everything to the buffer instead of inserting to the stream. Then the top level should work the same as you have in your CSV version.

[lukelowry]

@PhilipFackler I reworked this along the lines you suggested, so they all now use the same buffer. And added a consistency test. Thank you!

[PhilipFackler]

@lukelowry it looks like this PR now includes changes from #411. This will need to be fixed. I think this PR should touch only the VariableMonitor* files and possibly SystemModel.hpp

[lukelowry]

@lukelowry it looks like this PR now includes changes from #411. This will need to be fixed. I think this PR should touch only the VariableMonitor* files and possibly SystemModel.hpp

I think I messed up a rebase. I'll fix shortly, because I agree.

[lukelowry]

@PhilipFackler fixed, thank you. This speeds up tests a lot too

[lukelowry]

cc @nkoukpaizan incase slaven is busy

[pelesh]

I am getting following linker error:

[ 95%] Linking CXX executable test_phasor_system
Undefined symbols for architecture arm64:
  "GridKit::PhasorDynamics::BusBase<double, unsigned long>::getMonitor() const", referenced from:
      GridKit::Testing::SystemTests<double, unsigned long>::monitorOutputConsistency() in runSystemTests.cpp.o
ld: symbol(s) not found for architecture arm64
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [tests/UnitTests/PhasorDynamics/test_phasor_system] Error 1
make[1]: *** [tests/UnitTests/PhasorDynamics/CMakeFiles/test_phasor_system.dir/all] Error 2
make: *** [all] Error 2

@lukelowry, can you reproduce it at your end?

[lukelowry]

@pelesh thanks for catching this. It links on my Linux build, so I missed it.

Bus inherits getMonitor() from BusBase, but we only explicitly instantiated Bus, not BusBase. The new test exposed that missing BusBase<double, size_t>::getMonitor() symbol on macOS/arm64. I fixed it by explicitly instantiating BusBase next to the existing Bus instantiations.

I also removed the consistency test since it was only checking two wrappers over the same append path. Could you pull the update and confirm the arm64 build now links?

[PhilipFackler]

This still adds more complexity to the code than necessary. @lukelowry I'll send you a patch on slack that you can work with to integrate. I also sent via email. Let me know if you received it.

[lukelowry]

This still adds more complexity to the code than necessary. @lukelowry I'll send you a patch on slack that you can work with to integrate. I also sent via email. Let me know if you received it.

Patches applied!

[PhilipFackler]

Looks good. Great job!