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

Backport of cli: Optimize for large multi-line string outputs into v0.14 #27747

Conversation

teamterraform
Copy link
Contributor

Backport

This PR is auto-generated from #27746 to be assessed for backporting due to the inclusion of the label 0.14-backport.

The below text is copied from the body of the original PR.


When rendering a new, very large multi-line string output as part of terraform plan or terraform apply, we hit this code path:

if strings.Contains(val.AsString(), "\n") {
// It's a multi-line string, so we want to use the multi-line
// rendering so it'll be readable. Rather than re-implement
// that here, we'll just re-use the multi-line string diff
// printer with no changes, which ends up producing the
// result we want here.
// The path argument is nil because we don't track path
// information into strings and we know that a string can't
// have any indices or attributes that might need to be marked
// as (requires replacement), which is what that argument is for.
p.writeValueDiff(val, val, indent, nil)
break
}

The writeValueDiff multi-line string renderer calculates a longest common subsequence, which is an operation with quadratic time and space complexity. When the "two" strings are in fact the same value (as you can see from the call above), this results in a worst-case execution time. For very large strings, this can be several minutes:

$ time terraform-0.14.5 plan >/dev/null

real	4m3.381s
user	3m53.593s
sys	0m15.892s

This commit adds an early shortcut for this specific case, using simple string equality as the test, which directly prints the multi-line string without calculating an LCS. This dramatically improves the runtime in this specific edge case:

$ time terraform plan > /dev/null

real	0m0.104s
user	0m0.123s
sys	0m0.043s

Fixes #27743

@teamterraform teamterraform force-pushed the backport/alisdair/optimize-large-multi-line-string-outputs/precisely-feasible-glider branch 2 times, most recently from 8971dd8 to c16a976 Compare February 11, 2021 17:53
@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #27747 (f990eff) into v0.14 (a43338c) will decrease coverage by 0.00%.
The diff coverage is 87.50%.

Impacted Files Coverage Δ
command/format/diff.go 90.67% <87.50%> (+0.05%) ⬆️
internal/getproviders/types.go 72.53% <0.00%> (-1.41%) ⬇️
backend/remote/backend_common.go 51.98% <0.00%> (-0.73%) ⬇️
terraform/evaluate.go 52.89% <0.00%> (-0.42%) ⬇️

@alisdair alisdair merged commit cde0b24 into v0.14 Feb 11, 2021
@alisdair alisdair deleted the backport/alisdair/optimize-large-multi-line-string-outputs/precisely-feasible-glider branch February 11, 2021 18:46
@ghost
Copy link

ghost commented Mar 14, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants