Skip to content
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

jsonplan: Improve performance for deep objects #30561

Merged
merged 1 commit into from Feb 22, 2022

Conversation

alisdair
Copy link
Member

When calculating the unknown values for JSON plan output, we would previously recursively call the unknownAsBool function on the current sub-tree twice, if any values were unknown. This was wasteful, but not noticeable for normal Terraform resource shapes.

However for deeper nested object values, such as Kubernetes manifests, this was a severe performance problem, causing terraform show -json to take several hours to render a plan.

This commit reuses the already calculated unknown value for the subtree, and adds benchmark coverage to demonstrate the improvement.

Fixes #30548.

Benchmark results

For these benchmarks, we construct a fairly narrow object value of variable depth, then compute its unknown value. The execution time difference for this change is:

Object Depth Speedup
2 4x
3 7x
5 28x
7 116x
9 422x

Before:

$ go test -bench=. ./internal/command/jsonplan/
goos: darwin
goarch: amd64
pkg: github.com/hashicorp/terraform/internal/command/jsonplan
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkUnknownAsBool_2-16    	   10000	    101835 ns/op
BenchmarkUnknownAsBool_3-16    	    1947	    610288 ns/op
BenchmarkUnknownAsBool_5-16    	      48	  22955366 ns/op
BenchmarkUnknownAsBool_7-16    	       2	 836706698 ns/op
BenchmarkUnknownAsBool_9-16    	       1	27352356756 ns/op
PASS
ok  	github.com/hashicorp/terraform/internal/command/jsonplan	33.781s

After:

$ go test -bench=. ./internal/command/jsonplan/
goos: darwin
goarch: amd64
pkg: github.com/hashicorp/terraform/internal/command/jsonplan
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkUnknownAsBool_2-16    	   33733	     30257 ns/op
BenchmarkUnknownAsBool_3-16    	   13136	     90776 ns/op
BenchmarkUnknownAsBool_5-16    	    1458	    815009 ns/op
BenchmarkUnknownAsBool_7-16    	     162	   7209663 ns/op
BenchmarkUnknownAsBool_9-16    	      19	  64790729 ns/op
PASS
ok  	github.com/hashicorp/terraform/internal/command/jsonplan	9.800s

When calculating the unknown values for JSON plan output, we would
previously recursively call the `unknownAsBool` function on the current
sub-tree twice, if any values were unknown. This was wasteful, but not
noticeable for normal Terraform resource shapes.

However for deeper nested object values, such as Kubernetes manifests,
this was a severe performance problem, causing `terraform show -json` to
take several hours to render a plan.

This commit reuses the already calculated unknown value for the subtree,
and adds benchmark coverage to demonstrate the improvement.
@alisdair alisdair added cli performance 1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Feb 18, 2022
@alisdair alisdair requested a review from a team February 18, 2022 22:02
@alisdair alisdair self-assigned this Feb 18, 2022
Copy link
Member

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! Thanks for tracking this down.

@alisdair alisdair merged commit 1749228 into main Feb 22, 2022
@alisdair alisdair deleted the alisdair/json-output-optimization branch February 22, 2022 15:12
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged cli performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operation failed: failed generating plan JSON: failed running command (exit -1)
2 participants