feat: RBS size reduction#3382
Conversation
jterapin
left a comment
jterapin
left a comment
There was a problem hiding this comment.
Looking great so far. I'm curious to see the overall size reduction. Could you calculate the before vs after for all of our service gems? I think this will be great to highlight once this gets to prod.
As for the CodeQL failure, will this failure persist after this merge? If so, we should find a way to suppress (if not useful) or find a fix for it.
| return 0 if visited.include?(shape_name) | ||
|
|
||
| # Cache results to deduplicate calculation for nested structure types that get used multiple times in the model. | ||
| @size_cache ||= {} |
There was a problem hiding this comment.
Appears to be redundant since it was done at class init?
There was a problem hiding this comment.
Good catch, forgot to remove it after moving it off to initializer. Removed.
| def initialize(api:, shape:, newline:, options: {}) | ||
| def initialize(api:, shape:, newline:, options: {}, aliased_shapes: Set.new, alias_namespace: nil) | ||
| @api = api | ||
| @shape = shape | ||
| @newline = newline | ||
| @options = options | ||
| @aliased_shapes = aliased_shapes | ||
| @alias_namespace = alias_namespace |
There was a problem hiding this comment.
The signature are getting long here. We can do options = {} hash and deconstruct the has inside. Something like:
def initialize(options = {})
@api = options[:api]
...We have some examples on this pattern in our codebase.
There was a problem hiding this comment.
Good point, I just merged the two new fields into existing options field.
| if visited.include?(ref['shape']) | ||
| return "untyped" | ||
| else | ||
| visited = visited + [ref['shape']] | ||
| end |
There was a problem hiding this comment.
style:
return "untyped" if visited.include?(ref['shape'])
There was a problem hiding this comment.
Changed as recommended.
| ) | ||
| { | ||
| 'name' => Underscore.underscore(shape_name), | ||
| 'definition' => builder.struct(shape, ' ', []), |
There was a problem hiding this comment.
I'm not crazy with calling struct directly here. Looking at the prior version of KeywordArgumentBuilder, every external caller uses .format so I think .struct meant to be an internal detail so we could do the following:
def format_as_alias(indent: '')
struct(@shape, indent, [])
endBenefits of this:
struct,struct_members,ref_value, etc. can all move underprivate- Alias callers don't need to know about the visited array or indent format
- Clear two-method public API:
formatfor method signatures,format_as_alias fortype alias bodies
There was a problem hiding this comment.
Good point, changed as suggested (put private keyword below format_as_alias and format so all other methods are now private).
| shape: input_shape, | ||
| newline: true, | ||
| aliased_shapes: @aliased_shapes, | ||
| alias_namespace: 'Params', |
There was a problem hiding this comment.
I might be off base here, but it looks like alias_namespace is only ever 'Params' or nil so effectively functioning as a boolean flag. If so, could we do a simple boolean and default value will be false?
There was a problem hiding this comment.
Instead of substituting with a flag, I just removed that entirely and used string literal in the keyword argument builder when it swaps shape name with type alias.
| @@ -114,8 +114,17 @@ def rbs_files(options = {}) | |||
| prefix = options.fetch(:prefix, '') | |||
| codegenerated_plugins = codegen_plugins(prefix) | |||
| unless @service.h2_required_setting? | |||
There was a problem hiding this comment.
These changes are done for regular Client but what about AsyncClient?
There was a problem hiding this comment.
Oh oops, good catch. I didn't know Ruby published separate client type for bidirectional streaming lol. Added to async clients as well as pointed out.
2 participants
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Changes & Impact
aws-sdk-<service>/sig/folder in generated service gems by factoring out repeatedly used nested input struct definitions intosig/params.rbsas typealiases that get reused insig/client.rbs.sig/types.rbscannot be reused because that would make methods no longer accept hash literals.aws-sdk-quicksight. Itssig/folder at the time of this PR (aws-sdk-quicksight v1.179.0) was 35.5MB, but after this refactor it's reduced to 0.925MB, which is a reduction of about 97.3%. With other big services like EC2 and S3, reduction is much less due to those services primarily using flat input shape structure. EC2 gets about 5% reduction to itssig/(3.34MB => 3.17MB) and S3 gets about 1.5% reduction to itssig/(445KB => 439KB)....in RubyMine.InputTypeAliasCollectorwalks through shapes, caching names of shapes within operation input shapes that appear more than once & would end up taking more than 5 lines in rendered RBS if it were to be generated fully without typealias. It then returns topologically sorted shapes so that leaf shapes (shapes without dependencies) can get rendered first in RBS, allowing latter shapes to simply refer to leaf shapes in their own definitions.params.rbsto look at child typealias definitions anyways & couple extra type alises don't hurt, so additional complexity isn't worth it.