-
Notifications
You must be signed in to change notification settings - Fork 43
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
Further improvements in runtime metadata loading #1524
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1524 +/- ##
==========================================
+ Coverage 57.94% 57.97% +0.03%
==========================================
Files 286 286
Lines 39670 39687 +17
==========================================
+ Hits 22988 23010 +22
+ Misses 15342 15336 -6
- Partials 1340 1341 +1 ☔ View full report in Codecov by Sentry. |
85a5a62
to
0eb4188
Compare
0eb4188
to
34fc389
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good incremental step in load performance. LGTM
// to help reduce resource plugin start time | ||
// | ||
// See also pulumi/pulumi-terraform-bridge#1524 | ||
GenerateRuntimeMetadata bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why a provider would not want this flag enabled?
Put another way, if we were 100% confident that this would work as intended, would we enable it for all of our providers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not a lot of value for smaller providers, the data in the runtime config is strictly redundant with bridge-metadata.json (while being harder to read/edite) and it requires changing up the init a bit to use the runtime file for the runtime binary. So for now, I don't think it's worthwhile to try to enable this everywhere, lots of little frictions for minimal benefit on most providers.
We might revisit this as part of a larger effort to distinguish gentime/runtime though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me.
Co-authored-by: Ian Wahbe <[email protected]>
Enable the trimmed metadata from pulumi/pulumi-terraform-bridge#1524 to squeeze some milliseconds out of the provider init. ### Benchmarks ``` ❯ go test -bench "Provider$" -run "^$" -v -benchmem goos: darwin goarch: arm64 pkg: github.com/pulumi/pulumi-aws/provider/v6 BenchmarkProvider BenchmarkProvider-12 6 188479375 ns/op 331332504 B/op 2225300 allocs/op BenchmarkRuntimeProvider BenchmarkRuntimeProvider-12 8 141554162 ns/op 331336897 B/op 2225309 allocs/op ``` Note that these benchmarks are run with aliasing history generation enabled (we don't expose an api for disabling it to the provider), so they are slower than actual runtime performance.
Two changes to further reduce metadata load times for large providers:
In particular, removing extra whitespace contributed the most improvement suggesting that we may be able to obtain further savings just by shortening or eliminating frequently repeated strings.
This change is equivalent to the first codegen option described in pulumi/pulumi-aws#2799 (comment) (and combining the two yields no further improvement).
Generation of the runtime-only metadata is controlled via a flag in the ProviderInfo to allow providers to opt in individually.
Benchmarks from AWS:
Before changes
With zero-copy deserializer
With zero-copy deserializer and trimmed runtime only metadata