-
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
Test potential MustApplyAutoAliases speedup #1414
Conversation
pkg/tfbridge/auto_aliasing.go
Outdated
/* | ||
if err := md.Set(artifact, aliasMetadataKey, hist); err != nil { | ||
// Set fails only when `hist` is not serializable. Because `hist` is | ||
// composed of marshallable, non-cyclic types, this is impossible. | ||
contract.AssertNoErrorf(err, "History failed to serialize") | ||
} | ||
*/ |
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 change by itself yields:
❯ go test -run="^$" -bench="BenchmarkProvider$" -benchtime=10s
goos: darwin
goarch: arm64
pkg: github.com/pulumi/pulumi-aws/provider/v6
BenchmarkProvider-12 94 126865278 ns/op
PASS
Diff for pulumi-azuread with merge commit 37c59ac |
Diff for pulumi-random with merge commit 37c59ac |
Diff for pulumi-azuread with merge commit 8510cd5 |
Diff for pulumi-random with merge commit 8510cd5 |
Diff for pulumi-azuread with merge commit 874e28f |
Diff for pulumi-random with merge commit 874e28f |
b4e8dd1
to
62ced78
Compare
Diff for pulumi-azuread with merge commit b218473 |
Diff for pulumi-random with merge commit b218473 |
Diff for pulumi-random with merge commit 2b2be54 |
Diff for pulumi-azuread with merge commit 2b2be54 |
Codecov Report
@@ Coverage Diff @@
## master #1414 +/- ##
==========================================
+ Coverage 57.01% 57.84% +0.83%
==========================================
Files 147 284 +137
Lines 24400 39500 +15100
==========================================
+ Hits 13911 22849 +8938
- Misses 9505 15314 +5809
- Partials 984 1337 +353
... and 186 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Boo. Can't reproduce these test failures locally for some reason 😢 |
pkg/tfbridge/runtime.go
Outdated
} | ||
} | ||
|
||
func ReadRuntimeInfo() RuntimeInfo { |
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.
I'd be a bit careful exposing public API in pkg/tfbridge that's top visibility.
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.
Do we want providers to use this? Until there is a need for providers to use this functionality, I would keep it private (or internal/
).
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.
I was using it from aws to benchmark each way, but I agree we should not make it visible
Test failures : something around token tests is not recognizing the context it's running in I bet. 15% may be interesting. Let me see how far are we above the artificial alerting threshold. We need to go down 30% or so from the total. I'd be for taking some complexity here if it buys us 15% even if it doesn't get to where we need to be. |
Learned on a different thread that we may be initializing the plugin even more than 4 times in some programs; given that context, I agree with @t0yv0 that it's probably worth shipping this at least as a temporary measure until we have a chance to more carefully separate runtime and gen-time concerns. |
Diff for pulumi-random with merge commit 0920614 |
Diff for pulumi-azuread with merge commit 0920614 |
Diff for pulumi-azuread with merge commit e0c3359 |
Diff for pulumi-random with merge commit e0c3359 |
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.
LGTM
pkg/tfbridge/runtime.go
Outdated
func getRuntimeStage() runtimeStage { | ||
return runtime.stage | ||
} |
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.
Nit: I would inline this helper function.
pkg/tfbridge/auto_aliasing.go
Outdated
func isTfgen() bool { | ||
return getRuntimeStage() != resourceStage | ||
} |
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.
Nit: this function should be in the runtime.go
file.
pkg/tfbridge/auto_aliasing.go
Outdated
eHasH, eHasI := applyResourceMaxItemsOneAliasing(e, &h.Elem.Fields, &info.Elem.Fields) | ||
var eHasH, eHasI bool | ||
if h.Elem != nil { | ||
eHasH, eHasI = applyResourceMaxItemsOneAliasing(e, &h.Elem.Fields, &info.Elem.Fields) |
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.
Are we sure this here is safe? These functions edit SchemaInfo.MaxItemsOne to ensure backwards compatibility. If these edits are dropped at runtime, but only performed at compile-time, we'll get unexpected runtime serializer errors possibly.
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.
applyResourceMaxItemsOneAliasing
and applyMaxItemsOneAliasing
panic if h.Elem
is nil
The change in line 368 above means that sometimes h.elem
will be nil
here (ie. in the case that there was nothing in the metadata for this field), so we have to add the test. At runtime, it's safe to just skip because there's nothing in the metadata.
Although, it writing this I can see a cleaner way to handle this with a lot fewer conditionals, so I'll try that.
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.
I'm worried that this changes semantics. I don't know what conditions precisely get us to "sometimes h.elem will be nil" but it sounds like it shouldn't be/do that because it didn't trigger before the flags here were introduced?
Are we quite certain that under both versions of the flag isTfgen() = {true,false}
we're getting the same providerInfo back?
Something perhaps checkable with:
reflect.DeepEqual(a.Resources, b.Resouces)`
If this holds on a big provider I'd be less worried.
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.
Hm the last version looks a lot cleaner! It seems to me only history edits are skipped, and there's short-cicuiting of edits when we know there's no history so there definitionally shouldn't be edits. This looks safer. Think about the invariant above but if you're confident we don't need further testing.
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.
I'll run that test on aws before I merge, just to be sure.
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.
So the Resources are not quite identical, but I think semantically the same. When running with isTfGen() == false
we sometimes produce empty field maps, where the version with isTfgen() == true
produces nil
Diff:
--- Expected
+++ Actual
@@ -79007,4 +79007,3 @@
Elem: (*tfbridge.SchemaInfo)(<nil>),
- Fields: (map[string]*tfbridge.SchemaInfo) {
- },
+ Fields: (map[string]*tfbridge.SchemaInfo) <nil>,
Asset: (*tfbridge.AssetTranslation)(<nil>),
@@ -79124,4 +79123,3 @@
Elem: (*tfbridge.SchemaInfo)(<nil>),
- Fields: (map[string]*tfbridge.SchemaInfo) {
- },
+ Fields: (map[string]*tfbridge.SchemaInfo) <nil>,
Asset: (*tfbridge.AssetTranslation)(<nil>),
@@ -79168,4 +79166,3 @@
Elem: (*tfbridge.SchemaInfo)(<nil>),
- Fields: (map[string]*tfbridge.SchemaInfo) {
- },
+ Fields: (map[string]*tfbridge.SchemaInfo) <nil>,
Asset: (*tfbridge.AssetTranslation)(<nil>),
@@ -79212,4 +79209,3 @@
Elem: (*tfbridge.SchemaInfo)(<nil>),
- Fields: (map[string]*tfbridge.SchemaInfo) {
- },
+ Fields: (map[string]*tfbridge.SchemaInfo) <nil>,
Asset: (*tfbridge.AssetTranslation)(<nil>),
Test: TestProviderSame
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.
Yes, nil vs empty should be fine here.
|
||
var runtime = initRuntimeInfo() | ||
|
||
func initRuntimeInfo() runtimeInfo { |
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.
Beautiful.
Diff for pulumi-azuread with merge commit 52bd873 |
Diff for pulumi-random with merge commit 52bd873 |
Cleaned up the logic in |
toward #1410
Uses the build path for the binary to identify when we are in plugin runtime and disable sections of
ApplyAutoAliases
that are only needed for generating the bridge metadata file.I've extracted the bit of trying to guess whether we're in a tfgen binary or a plugin binary to it's own module so we could change out the implementation more easily in the future.