Skip to content

some performance improvements - #586

Open
superwhiskers wants to merge 1 commit into
developfrom
performance-improvements
Open

superwhiskers wants to merge 1 commit into
developfrom
performance-improvements

Conversation

@superwhiskers

@superwhiskers superwhiskers commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

this pull request provides some performance improvements that target larger cases, like activsg10k.

as a quick, unscientific check,

  • application/PhasorDynamics/DynamicSimulation on ACTIVSg10k completes in 24.3332 seconds using these changes
  • application/PhasorDynamics/DynamicSimulation on ACTIVSg10k completes in 51.6972 seconds as of 9330ee88

both builds were a standard release (RelWithDebInfo) build

Checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.
  • N/A The CHANGELOG.md has been updated to reflect the changes. If this is a minor PR that is part of a larger fix already included in the file, state so.

@superwhiskers
superwhiskers force-pushed the performance-improvements branch 4 times, most recently from 111df0f to fe4d44c Compare September 18, 2026 19:47
@pelesh
pelesh removed the request for review from PhilipFackler September 21, 2026 14:45
@pelesh

pelesh commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Fails to build with following error message:

[ 75%] Building CXX object tests/UnitTests/PhasorDynamics/CMakeFiles/test_phasor_governor_tgov1.dir/runGovernorTgov1Tests.cpp.o
Undefined symbols for architecture arm64:
  "void GridKit::LinearAlgebra::detail::logBoundsCheckFailure<unsigned long>(unsigned long, unsigned long)", referenced from:
      GridKit::Testing::VectorTests<double, unsigned long>::setToConst(unsigned long) in runVectorTests.cpp.o
      GridKit::Testing::VectorTests<double, unsigned long>::setToConst(unsigned long) in runVectorTests.cpp.o
      GridKit::Testing::VectorTests<double, unsigned long>::setToConst(unsigned long) in runVectorTests.cpp.o
      GridKit::Testing::VectorTests<double, unsigned long>::setToConst(unsigned long) in runVectorTests.cpp.o
      GridKit::Testing::VectorTests<double, unsigned long>::setToConst(unsigned long) in runVectorTests.cpp.o
      GridKit::Testing::VectorTests<double, unsigned long>::setToConst(unsigned long) in runVectorTests.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/LinearAlgebra/Vector/test_vector] Error 1
make[1]: *** [tests/UnitTests/LinearAlgebra/Vector/CMakeFiles/test_vector.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 75%] Built target test_vector_handler
[ 75%] Built target test_math_smoothness_indicator
[ 75%] Built target test_phasor_bus
[ 75%] Built target phasor_dynamics_gensal_dependency_tracking
[ 75%] Linking CXX executable test_phasor_bus_fault
[ 75%] Built target test_phasor_bus_fault

@pelesh pelesh added the enhancement New feature or request label Sep 21, 2026
@pelesh
pelesh requested a review from lukelowry September 21, 2026 15:04
@pelesh pelesh added this to the Release 0.2 milestone Sep 21, 2026
@pelesh

pelesh commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Same error with a clean build from scratch.

@superwhiskers
superwhiskers force-pushed the performance-improvements branch from fe4d44c to ca40c09 Compare September 21, 2026 19:27
@superwhiskers
superwhiskers marked this pull request as ready for review September 21, 2026 19:29

@pelesh pelesh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some nitpicking comments to consider.

Comment thread GridKit/LinearAlgebra/Vector/Vector.hpp Outdated
Comment thread GridKit/Solver/Dynamic/Ida.cpp Outdated
@pelesh

pelesh commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

10k system validation time is reduced from 26.9s to 20.4s on my machine.

@pelesh

pelesh commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Unit tests for the vector class all pass.

@nkoukpaizan nkoukpaizan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!

10K solution time dropped from ~60s to ~45 on Frontier (single thread CPU). Performance on my MacOS did not change significantly (~30s). I suspect different default inlining behaviors. This is overall a net benefit.

@superwhiskers I recommend updating the PR description to reflect the final speedups you saw on your end.

Comment on lines -25 to -26
Log::misc() << "Evaluate Jacobian for Tgov1..." << std::endl;
Log::misc() << "Jacobian evaluation is experimental!" << std::endl;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are removing these, there should be one more in TgovEnzymeDependencyTracking.cpp for "Jacobian evaluation is experimental!".

Could you comment on motivation for removing all of them? We don't really need them anymore, but the \n versions should not significantly impact performance. Also see #512 (out of scope for this PR).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's something i thought i'd try and while it didn't influence anything, i thought i'd leave it in because it seemed unnecessary

@lukelowry lukelowry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faster=happier

Thank you good work

@superwhiskers
superwhiskers force-pushed the performance-improvements branch from a277768 to 1d63e1c Compare September 21, 2026 20:58
@lukelowry
lukelowry force-pushed the performance-improvements branch 2 times, most recently from 8f68d89 to 1d63e1c Compare September 22, 2026 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants