Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize TestSuite comparison macro compile times. #140

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mosra
Copy link
Owner

@mosra mosra commented Jun 1, 2022

I regularly get angry at how long large tests take to compile. Well, I guess relatively, building a heavy 6000-line GltfImporterTest.cpp in under 5 seconds is an unachievable feat in Some Other Codebases. But still.

Profiling using Clang -ftime-trace, the biggest offenders are:

  • parsing the <sstream> include, about 160 ms / 3.5% -- depends on making Debug stream-free, which depends on ... making my own float printers
  • parsing <cmath>, about 40 ms / 1% -- can't really do much about this, and the more I optimize, the more this will stand out
  • template instantiations, about 1000 ms / 20%, of which the biggest chunks are:
    • TestSuite::Compare::Container<T>::printMessage(), about 500 ms / 10% -- put it into a non-templated base. These are not so many but quite large.
      • The comparator also does the comparisons twice on error, which is fine at runtime but adds even more pain to compile times. Why not just remember the first different item instead, instead of looping through everything again?!
    • TestSuite::Comparator<T>::printMessage() -- put it into a non-templated base. These are tiny but quite many, so the template base might not help as much.

image

Comparison with SpeedScope after adding a non-templated base for both Comparator and Compare::Container is above -- 4.9 seconds before, 4.5 after, time spent in printMessage() visibly shrinks. Trace files for reference:

10% reduction in compile time is still not as significant as I hoped, further investigation needed:

  • The actual container operator<<() seems to be quite heavy, anything to optimize there?
  • What else?

Majority of the work done in this class is template-independent, so this
avoid a lot of needless template instantiations.
mosra added 2 commits June 1, 2022 10:38
Since this is a failure case, it doesn't really matter if it takes 10x
more time at runtime. But it definitely adds more work for the compiler,
and that's where most of human time is usually spent -- iterating on the
test code. So don't.
Same as was done for the generic Comparator, but here it's quite a lot
heavier so the impact should be bigger.
@mosra mosra force-pushed the testsuite-compile-times branch from 804de7e to 468afdb Compare June 1, 2022 09:32
@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #140 (468afdb) into master (c5102b3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #140   +/-   ##
=======================================
  Coverage   98.40%   98.40%           
=======================================
  Files         124      125    +1     
  Lines        9504     9541   +37     
=======================================
+ Hits         9352     9389   +37     
  Misses        152      152           
Impacted Files Coverage Δ
src/Corrade/TestSuite/Comparator.cpp 100.00% <100.00%> (ø)
src/Corrade/TestSuite/Comparator.h 100.00% <100.00%> (ø)
src/Corrade/TestSuite/Compare/Container.cpp 100.00% <100.00%> (ø)
src/Corrade/TestSuite/Compare/Container.h 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5102b3...468afdb. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

1 participant