Repository navigation
Conversation
**Context** Today users of vtproto benefit from fully unrolled ser/des but cannot take advantage of pooling, creating GC churn where there would otherwise be none. This diff: * Adds pooling support to the generator for well-known types. * Updates Makefile w/ pooling arguments for gen-wkt and gen-testproto * Adds additional fields to WKTNestedPool test type for testing * Regenerates test wkt and test protos * Adds IsLocalWrapper helper func to generator * Adds new unit tests exercising cloning behavior The new pooling code is inert for existing stubs; as such this change should be backwards compatible. Users can leverage this by regenerating their stubs. **⚠️ Caveats⚠️ ** While the code is backwards compatible, this is a _material change in behavior_. Previously there were cases where calling `ResetVT` on a type containing a singular WKT would simply clear the pointer. This meant it was safe to share/reuse WKT objects in certain cases. This is no longer true; all the same terms and conditions that apply to other pooled types apply to WKT wrappers. Currently Struct's do not benefit much from this as we do not have pooling for maps. This will be addressed in a follow on change.
This diff refactors planetscale#156 to add support for pooling maps. **Caveats** * Just like with pooled slices, pooled maps do not shrink. This is a footgun we may want to document explicitly.
This diff regenerates the wkt wrappers with map pooling support.
This diff updates the generated unmarshal code to pull map values from pools when pooling is enabled for both parent messages and the value type. **Caveats** * If a given key is repeated, the old pooled value will be dropped and picked up by GC. This is done for simplicity. * If unmarshal fails partway through, any values pulled from the pool will be left for GC. This behavior matches what happens with oneof's.
Author
|
@mattlord I'd appreciate some eyes on this whenever you get a chance <3 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This PR makes three changes to the generator:
The commits reflect the changes in the order above. I elected to bundle them because all three changes are small and related. I'm happy to break these into separate PRs if folks feel that's appropriate. I'm also happy to make any doc updates that seem relevant.