-
Notifications
You must be signed in to change notification settings - Fork 108
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
Utility: deduplicate __FILE__ in internal assertion macros #182
Draft
mosra
wants to merge
3
commits into
master
Choose a base branch
from
assert-deduplicate-file
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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? - 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). - I had an idea to insert `\0`s 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.
This is one less function call, however it doesn't really have any effect either. The total change with libMagnumWhee.so is now this: FILE SIZE VM SIZE -------------- -------------- +3.1% +9.92Ki +3.1% +9.92Ki .text +155% +3.65Ki [ = ] 0 [Unmapped] +0.6% +368 +0.6% +368 .eh_frame +0.1% +80 +0.1% +80 .dynstr +0.5% +64 +0.5% +64 .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 -0.3% -46 -0.3% -46 .gcc_except_table -5.5% -14.1Ki -5.5% -14.1Ki .rodata [ = ] 0 -0.4% -3.65Ki TOTAL
This further reduces the string table, but doesn't help in making libMagnumWhee.so any smaller either. Plus a consequence is that using an assertion pulls in all the number-to-string conversion machinery, which is absolutely undesitable. FILE SIZE VM SIZE -------------- -------------- +2.9% +9.38Ki +2.9% +9.38Ki .text +218% +5.13Ki [ = ] 0 [Unmapped] +0.6% +368 +0.6% +368 .eh_frame +0.1% +80 +0.1% +80 .dynstr +0.5% +64 +0.5% +64 .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 -0.3% -46 -0.3% -46 .gcc_except_table -5.9% -15.0Ki -5.9% -15.0Ki .rodata [ = ] 0 -0.6% -5.13Ki TOTAL
Codecov ReportAttention: Patch coverage is
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 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
andwc -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:Testing with libMagnumTrade.so, which has 8 kB of internal assertions, and ~2 kB after, didn't show any meaningful change however:
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.
The conclusion is that this change is probably not worth doing in its current form. Putting aside until I get a better idea.
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."Assertion {} failed at {}:{}"
, condition, file, and pass line as a number?)\0
s 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.