Simplify sync::Transformer and eliminate the abstract interface #7099
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
Pull Request Test Coverage Report for Build github_pull_request_282307
💛 - Coveralls |
transform.cpp uses the magic of templates to generate a very, very large number of functions, and many of them were unnecessarily extern. This resulted in the symbol table being over half of transform.o in Release builds. Making more of them private cuts the arm64 Release build from 250 KB to 128 KB.
Transformer objects used to have some local state which had to be stored between uses and had the implementation hidden in the cpp file. Neither of those are true any more, so the design doesn't make sense either. It can now just be a stack-allocated local created when needed rather than something which has to be stored and reused.
These were required prior to dangling links.
danieltabacaru
approved these changes
Nov 1, 2023
michael-wb
approved these changes
Nov 2, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
https://github.com/realm/realm-sync/pull/3196 pulled TransformerImpl out of the anonymous namespace and made it extern. This innocent-looking change actually had a meaningful effect: a very, very large number of functions defined by composing templates were no longer inlined out of existence because static functions are inlined more aggressively than extern functions. Moving all of the helper functions back to the anonymous namespace results in transform.o halving in size due to better inlining.
While poking at this I noticed that Transformer's API didn't make any sense any more. It was designed to support Transformer objects holding state between transformations and to hide the implementation type entirely in the cpp file. Neither of these are done any more, so it was just very overcomplicated for no benefit.
This cuts ~150 KB off the obj-c dylib and makes the sync transform benchmark 5-10% faster, which doesn't seem big enough to be worth mentioning in the changelog.