corrade icon indicating copy to clipboard operation
corrade copied to clipboard

Utility: deduplicate __FILE__ in internal assertion macros

Open mosra opened this issue 1 year ago • 1 comments

Previously, it was joined together with the actual stringified condition, leading to the file path being repeated several times in the resulting binary. By making it a separate literal it has a null terminator, which makes the compiler able to deduplicate all such occurences to just one.

In a stripped libCorradeUtility.so this results in the binary getting about 4 kB smaller (641 kB before, 637 after). Looking with strings, grep and wc -c, the internal assertion messages were about 8 kB before, now they're together with deduplicated filenames ~4 kB. Detailed Bloaty report however shows that the actual code size increased significantly as well, offseting the gains:

    FILE SIZE        VM SIZE
 --------------  --------------
  +0.7% +2.53Ki  +0.7% +2.53Ki    .text
  [ = ]       0   +33%      +8    .data
  -0.1%      -8  -0.1%      -8    .eh_frame_hdr
  -0.2%      -8  -0.2%      -8    .got.plt
  -0.1%     -12  -0.1%     -12    .gcc_except_table
  -0.0%     -16  -0.0%     -16    .dynstr
  -0.2%     -16  -0.2%     -16    .plt
  -0.1%     -24  -0.1%     -24    .dynsym
  -0.2%     -24  -0.2%     -24    .rela.plt
  -0.1%     -64  -0.1%     -64    .eh_frame
 -22.9% -1.81Ki  [ = ]       0    [Unmapped]
 -14.3% -4.56Ki -14.3% -4.56Ki    .rodata
  -0.6% -4.01Ki  -0.3% -2.19Ki    TOTAL

Testing with libMagnumTrade.so, which has 8 kB of internal assertions, and ~2 kB after, didn't show any meaningful change however:

    FILE SIZE        VM SIZE
 --------------  --------------
  +0.7% +4.00Ki  +0.7% +4.00Ki    .text
   +34% +1.72Ki  [ = ]       0    [Unmapped]
  -6.4% -5.72Ki  -6.4% -5.72Ki    .rodata
  [ = ]       0  -0.2% -1.72Ki    TOTAL

And neither did with libMagnumWhee.so, the new UI library replacement, where internal assertions were 27 kB (!) before, and ~12 kB after. All the gains were eaten up by increases in code size, and what I suspect is two extra 64-bit pointers for the new strings in each assertions.

    FILE SIZE        VM SIZE
 --------------  --------------
  +2.7% +8.72Ki  +2.7% +8.72Ki    .text
  +223% +5.24Ki  [ = ]       0    [Unmapped]
  +0.4%    +264  +0.4%    +264    .eh_frame
  +0.1%     +80  +0.1%     +80    .dynstr
  +0.4%     +48  +0.4%     +48    .eh_frame_hdr
  +0.1%     +24  +0.1%     +24    .dynsym
  +0.1%      +8  +0.1%      +8    .gnu.hash
  +0.3%      +8  +0.3%      +8    .gnu.version
  -1.1%    -197  -1.1%    -197    .gcc_except_table
  -5.6% -14.2Ki  -5.6% -14.2Ki    .rodata
  [ = ]       0  -0.6% -5.24Ki    TOTAL

The conclusion is that this change is probably not worth doing in its current form. Putting aside until I get a better idea.

  • [ ] Maybe a radical simplification of the Debug helper (with a fast path for printing just char*) would help?
  • [ ] Maybe splitting to just two strings instead of 3 (assertion message, file, line) could help, and letting the internals format it somehow?
    • [x] ~~Or two strings + a line as a number, which then gets converted to a string at runtime?~~ Tried, doesn't make it any better either, and makes the printing dependent on the whole number-to-string machinery.
    • [ ] Or maybe keep 3 but deduplicate the template as well? ("Assertion {} failed at {}:{}", condition, file, and pass line as a number?)
  • [ ] Maybe just sidestep Debug altogether in these and just std::puts() or whatever? Most of the formatting functionality isn't needed anyway, all we have is char pointers, although it should still be capable of redirecting default output to somewhere else (such as adb logcat on Android).
  • [x] I had an idea to insert \0s in the literal, remember the literal size and then split them up internally, but that would only allow deduplication with __FILE__ being used standalone somewhere else, not any other asserts.

mosra avatar Jul 31 '24 10:07 mosra

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.28%. Comparing base (f966f91) to head (ba2e85c). Report is 311 commits behind head on master.

Files with missing lines Patch % Lines
src/Corrade/Utility/Assert.h 50.00% 1 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #182   +/-   ##
=======================================
  Coverage   98.28%   98.28%           
=======================================
  Files         141      141           
  Lines       12706    12706           
=======================================
  Hits        12488    12488           
  Misses        218      218           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jul 31 '24 15:07 codecov[bot]