Ruby: Use class inheritance to save memory #10281
Merged
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
|
I wish there was a way to do this that didn't affect the public API. If we do this, developers can start writing: if obj.kind_of? Google::Protobuf::AbstractMessage
# This is a protobuf
endThat might not be the worst thing, but it would commit us to maintaining this inheritance hierarchy. |
|
I can make that constant private. |
The only difference between all the `Message` classes is just their `descriptor` instance variable. So rather than create an entirely new class from scratch every time we can simply inherit from an abstract class. This shink each `Message` class from `1456` bytes to `944` bytes, and the singleton class of each from `960` to `792`, for a total of `680` bytes saved per message class, so a ~28% reduction.
6c5cacb to
7309e88
Compare
|
Done: >> require 'google/protobuf'
=> true
>> Google::Protobuf::AbstractMessage
(irb):2:in `<main>': private constant Google::Protobuf::AbstractMessage referenced (NameError) |
haberman
approved these changes
Aug 10, 2022
jez
added a commit
to jez/protoc-gen-rbi
that referenced
this pull request
Apr 26, 2023
In our codebase we're fighting a constant battle against the codebase's growing size. Having a larger codebase makes basically everything slower, from creating an initial dev environment to type checking code to creating deploy artifacts. Generated protobuf files end up contributing a lot to the bloat of our codebase. These two options make protoc-gen-rbi generate less code. The options: - `hide_common_methods` This option hides the common methods like `decode_json` and `to_h` that protoc-gen-rbi would otherwise create. While it's most accurate to have those methods be defined on these classes, it's actually possible to get basically the same type safety by defining these methods elsewhere, using `T.attached_class`. - `use_abstract_message` A recent `google-protobuf` PR changed the superclass of all message classes: <protocolbuffers/protobuf#10281> The new superclass, `Google::Protobuf::AbstractMessage`, is technically private. However, having a common superclass is quite convenient for a number of reasons, and codebases may wish forcibly make this class publicly visible. If that's been done, it's nice to be able to mention that superclass instead of the `include` and `extend` calls, because `AbstractMessage` is exactly defined by just an empty class with those two mixins, and getting rid of those calls generates less code. For more discussions on the merits of making this class public: <protocolbuffers/protobuf#12550> I'm happy to split this change apart if you think it would be better that way. On Stripe's codebase, these changes amount to a 4% reduction in Sorbet's peak memory usage, and a 1% improvement to time spent type checking, so I'm quite eager to get this change or some equivalent version of it landed.
jez
added a commit
to jez/protoc-gen-rbi
that referenced
this pull request
Apr 26, 2023
In our codebase we're fighting a constant battle against the codebase's growing size. Having a larger codebase makes basically everything slower, from creating an initial dev environment to type checking code to creating deploy artifacts. Generated protobuf files end up contributing a lot to the bloat of our codebase. These two options make protoc-gen-rbi generate less code. The options: - `hide_common_methods` This option hides the common methods like `decode_json` and `to_h` that protoc-gen-rbi would otherwise create. While it's most accurate to have those methods be defined on these classes, it's actually possible to get basically the same type safety by defining these methods elsewhere, using `T.attached_class`. - `use_abstract_message` A recent `google-protobuf` PR changed the superclass of all message classes: <protocolbuffers/protobuf#10281> The new superclass, `Google::Protobuf::AbstractMessage`, is technically private. However, having a common superclass is quite convenient for a number of reasons, and codebases may wish forcibly make this class publicly visible. If that's been done, it's nice to be able to mention that superclass instead of the `include` and `extend` calls, because `AbstractMessage` is exactly defined by just an empty class with those two mixins, and getting rid of those calls generates less code. For more discussions on the merits of making this class public: <protocolbuffers/protobuf#12550> I'm happy to split this change apart if you think it would be better that way. On Stripe's codebase, these changes amount to a 4% reduction in Sorbet's peak memory usage, and a 1% improvement to time spent type checking, so I'm quite eager to get this change or some equivalent version of it landed.
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.
The only difference between all the
Messageclasses is just theirdescriptorinstance variable.So rather than create an entirely new class from scratch every time we can simply inherit from an abstract class.
This shink each
Messageclass from1456bytes to944bytes, and the singleton class of each from960to792, for a total of680bytes saved per message class, so a ~28% reduction.