Fix mustWrite for Input/Output PropertyMaps (#6396)

CHANGELOG.md

@@ -32,6 +32,9 @@ _(none)_
 - [sdk/nodejs] Don't error when loading multiple copies of the same version of a Node.js
   component package. [#6387](https://github.com/pulumi/pulumi/pull/6387)
 
+- [cli] Skip unnecessary state file writes to address performance regression introduced in 2.16.2.
+  [#6396](https://github.com/pulumi/pulumi/pulls/6396)
+
 ## 2.21.1 (2021-02-18)
 
 ### Bug Fixes

pkg/backend/snapshot.go

@@ -181,22 +181,26 @@ func (ssm *sameSnapshotMutation) mustWrite(step *deploy.SameStep) bool {
 	// If the URN of this resource has changed, we must write the checkpoint. This should only be possible when a
 	// resource is aliased.
 	if old.URN != new.URN {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of URN")
 		return true
 	}
 
 	// If the type of this resource has changed, we must write the checkpoint. This should only be possible when a
 	// resource is aliased.
 	if old.Type != new.Type {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of Type")
 		return true
 	}
 
 	// If the kind of this resource has changed, we must write the checkpoint.
 	if old.Custom != new.Custom {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of Custom")
 		return true
 	}
 
 	// We need to persist the changes if CustomTimes have changed
 	if old.CustomTimeouts != new.CustomTimeouts {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of CustomTimeouts")
 		return true
 	}
 
@@ -205,23 +209,31 @@ func (ssm *sameSnapshotMutation) mustWrite(step *deploy.SameStep) bool {
 	// If this resource's provider has changed, we must write the checkpoint. This can happen in scenarios involving
 	// aliased providers or upgrades to default providers.
 	if old.Provider != new.Provider {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of Provider")
 		return true
 	}
 
 	// If this resource's parent has changed, we must write the checkpoint.
 	if old.Parent != new.Parent {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of Parent")
 		return true
 	}
 
 	// If the protection attribute of this resource has changed, we must write the checkpoint.
 	if old.Protect != new.Protect {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of Protect")
 		return true
 	}
 
 	// If the inputs or outputs of this resource have changed, we must write the checkpoint. Note that it is possible
 	// for the inputs of a "same" resource to have changed even if the contents of the input bags are different if the
 	// resource's provider deems the physical change to be semantically irrelevant.
-	if !reflect.DeepEqual(old.Inputs, new.Inputs) || !reflect.DeepEqual(old.Outputs, new.Outputs) {
+	if !old.Inputs.DeepEquals(new.Inputs) {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of Inputs")
+		return true
+	}
+	if !old.Outputs.DeepEquals(new.Outputs) {
+		logging.V(9).Infof("SnapshotManager: mustWrite() true because of Outputs")
 		return true
 	}
 
@@ -235,6 +247,7 @@ func (ssm *sameSnapshotMutation) mustWrite(step *deploy.SameStep) bool {
 	// lists being empty ourselves.
 	if len(old.Dependencies) != 0 || len(new.Dependencies) != 0 {
 		if !reflect.DeepEqual(old.Dependencies, new.Dependencies) {
+			logging.V(9).Infof("SnapshotManager: mustWrite() true because of Dependencies")
 			return true
 		}
 	}
@@ -242,6 +255,7 @@ func (ssm *sameSnapshotMutation) mustWrite(step *deploy.SameStep) bool {
 	// Init errors are strictly advisory, so we do not consider them when deciding whether or not to write the
 	// checkpoint.
 
+	logging.V(9).Infof("SnapshotManager: mustWrite() false")
 	return false
 }
 
@@ -275,6 +289,7 @@ func (ssm *sameSnapshotMutation) End(step deploy.Step, successful bool) error {
 			return false
 		}
 
+		logging.V(9).Infof("SnapshotManager: sameSnapshotMutation.End() not eliding write")
 		return true
 	})
 }

pkg/backend/snapshot_test.go

@@ -21,10 +21,13 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"github.com/pulumi/pulumi/pkg/v2/resource/deploy"
+	"github.com/pulumi/pulumi/pkg/v2/resource/stack"
 	"github.com/pulumi/pulumi/pkg/v2/secrets"
 	"github.com/pulumi/pulumi/pkg/v2/secrets/b64"
 	"github.com/pulumi/pulumi/pkg/v2/version"
 	"github.com/pulumi/pulumi/sdk/v2/go/common/resource"
+	"github.com/pulumi/pulumi/sdk/v2/go/common/resource/config"
+	"github.com/pulumi/pulumi/sdk/v2/go/common/resource/plugin"
 	"github.com/pulumi/pulumi/sdk/v2/go/common/tokens"
 )
 
@@ -72,6 +75,16 @@ func NewResourceWithDeps(name string, deps []resource.URN) *resource.State {
 	}
 }
 
+func NewResourceWithInputs(name string, inputs resource.PropertyMap) *resource.State {
+	return &resource.State{
+		Type:         tokens.Type("test"),
+		URN:          resource.URN(name),
+		Inputs:       inputs,
+		Outputs:      make(resource.PropertyMap),
+		Dependencies: []resource.URN{},
+	}
+}
+
 func NewResource(name string, deps ...resource.URN) *resource.State {
 	return NewResourceWithDeps(name, deps)
 }
@@ -134,6 +147,33 @@ func TestSamesWithEmptyDependencies(t *testing.T) {
 	assert.Len(t, sp.SavedSnapshots, 0, "expected no snapshots to be saved for same step")
 }
 
+func TestSamesWithEmptyArraysInInputs(t *testing.T) {
+	// Model reading from state file
+	state := map[string]interface{}{"defaults": []interface{}{}}
+	inputs, err := stack.DeserializeProperties(state, config.NopDecrypter, config.NopEncrypter)
+	assert.NoError(t, err)
+
+	res := NewResourceWithInputs("a-unique-urn-resource-a", inputs)
+	snap := NewSnapshot([]*resource.State{
+		res,
+	})
+	manager, sp := MockSetup(t, snap)
+
+	// Model passing into and back out of RPC layer (e.g. via `Check`)
+	marshalledInputs, err := plugin.MarshalProperties(inputs, plugin.MarshalOptions{})
+	assert.NoError(t, err)
+	inputsUpdated, err := plugin.UnmarshalProperties(marshalledInputs, plugin.MarshalOptions{})
+	assert.NoError(t, err)
+
+	resUpdated := NewResourceWithInputs(string(res.URN), inputsUpdated)
+	same := deploy.NewSameStep(nil, nil, res, resUpdated)
+	mutation, err := manager.BeginMutation(same)
+	assert.NoError(t, err)
+	err = mutation.End(same, true)
+	assert.NoError(t, err)
+	assert.Len(t, sp.SavedSnapshots, 0, "expected no snapshots to be saved for same step")
+}
+
 // This test challenges the naive approach of mutating resources
 // that are the targets of Same steps in-place by changing the dependencies
 // of two resources in the snapshot, which is perfectly legal in our system