From 04f1d062d6203436f78b81d66549735ebb6152cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leandro=20L=C3=B3pez?= Date: Wed, 4 Jan 2023 17:28:51 -0300 Subject: [PATCH] Improve plan summary performance (#2907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add benchmark for *PlanSuccess.Summary This is for taking a baseline and comparing with future improvements. I just did a test run and got the following results: $ go test -bench=. ./server/events/command goos: darwin goarch: arm64 pkg: github.com/runatlantis/atlantis/server/events/command BenchmarkPlanSuccess_Summary/empty_summary,_no_matches-10 82564 12415 ns/op 27352 B/op 114 allocs/op BenchmarkPlanSuccess_Summary/changes-10 167904 7148 ns/op 14309 B/op 69 allocs/op BenchmarkPlanSuccess_Summary/no_changes-10 92410 12984 ns/op 27705 B/op 114 allocs/op BenchmarkPlanSuccess_Summary/changes_outside_Terraform-10 89256 13505 ns/op 27882 B/op 117 allocs/op BenchmarkPlanSuccess_Summary/changes_and_changes_outside-10 159199 7527 ns/op 14493 B/op 72 allocs/op PASS ok github.com/runatlantis/atlantis/server/events/command 7.536s To have a better comparison I've ran all the benchmarks with 200,000 iterations, and got this: $ go test -bench=. ./server/events/command -benchtime=200000x goos: darwin goarch: arm64 pkg: github.com/runatlantis/atlantis/server/events/command BenchmarkPlanSuccess_Summary/changes_and_changes_outside-10 200000 7637 ns/op 14487 B/op 72 allocs/op BenchmarkPlanSuccess_Summary/empty_summary,_no_matches-10 200000 12379 ns/op 27352 B/op 114 allocs/op BenchmarkPlanSuccess_Summary/changes-10 200000 7134 ns/op 14312 B/op 69 allocs/op BenchmarkPlanSuccess_Summary/no_changes-10 200000 12960 ns/op 27692 B/op 114 allocs/op BenchmarkPlanSuccess_Summary/changes_outside_Terraform-10 200000 13731 ns/op 27862 B/op 117 allocs/op PASS ok github.com/runatlantis/atlantis/server/events/command 11.837s Signed-off-by: Leandro López (inkel) * Compile summary regexps only once By initializing these regexes at program first run instead of every time the function is called we gain better resource usage and much faster results, as shown in this results with 200,000 iterations as in the previous commit: $ go test -bench=. ./server/events/command -benchtime=200000x goos: darwin goarch: arm64 pkg: github.com/runatlantis/atlantis/server/events/command BenchmarkPlanSuccess_Summary/empty_summary,_no_matches-10 200000 29.49 ns/op 0 B/op 0 allocs/op BenchmarkPlanSuccess_Summary/changes-10 200000 450.4 ns/op 0 B/op 0 allocs/op BenchmarkPlanSuccess_Summary/no_changes-10 200000 357.9 ns/op 0 B/op 0 allocs/op BenchmarkPlanSuccess_Summary/changes_outside_Terraform-10 200000 824.1 ns/op 193 B/op 3 allocs/op BenchmarkPlanSuccess_Summary/changes_and_changes_outside-10 200000 754.8 ns/op 177 B/op 3 allocs/op PASS ok github.com/runatlantis/atlantis/server/events/command 0.943s With 200,000 iterations the results were almost immediately and I couldn't observe the real performance, so I've removed the limitation and got the following: $ go test -bench=. ./server/events/command -benchtime=200000x goos: darwin goarch: arm64 pkg: github.com/runatlantis/atlantis/server/events/command BenchmarkPlanSuccess_Summary/empty_summary,_no_matches-10 200000 29.49 ns/op 0 B/op 0 allocs/op BenchmarkPlanSuccess_Summary/changes-10 200000 450.4 ns/op 0 B/op 0 allocs/op BenchmarkPlanSuccess_Summary/no_changes-10 200000 357.9 ns/op 0 B/op 0 allocs/op BenchmarkPlanSuccess_Summary/changes_outside_Terraform-10 200000 824.1 ns/op 193 B/op 3 allocs/op BenchmarkPlanSuccess_Summary/changes_and_changes_outside-10 200000 754.8 ns/op 177 B/op 3 allocs/op PASS ok github.com/runatlantis/atlantis/server/events/command 0.943s I've also compared the results of running the benchamrks with 200,000 iterations, 10 times each, and these are the results: $ benchstat orig.txt improv.txt name old time/op new time/op delta PlanSuccess_Summary/empty_summary,_no_matches-10 12.6µs ± 2% 0.0µs ± 1% -99.90% (p=0.000 n=10+9) PlanSuccess_Summary/changes-10 7.19µs ± 0% 0.36µs ± 2% -94.96% (p=0.000 n=8+9) PlanSuccess_Summary/no_changes-10 13.2µs ± 2% 0.4µs ± 0% -97.27% (p=0.000 n=10+9) PlanSuccess_Summary/changes_outside_Terraform-10 13.7µs ± 1% 0.8µs ± 0% -94.02% (p=0.000 n=9+9) PlanSuccess_Summary/changes_and_changes_outside-10 7.71µs ± 5% 0.75µs ± 0% -90.21% (p=0.000 n=10+8) name old alloc/op new alloc/op delta PlanSuccess_Summary/empty_summary,_no_matches-10 27.4kB ± 0% 0.0kB -100.00% (p=0.000 n=10+10) PlanSuccess_Summary/changes-10 14.3kB ± 0% 0.0kB -100.00% (p=0.000 n=9+10) PlanSuccess_Summary/no_changes-10 27.7kB ± 0% 0.0kB -100.00% (p=0.000 n=10+10) PlanSuccess_Summary/changes_outside_Terraform-10 27.9kB ± 0% 0.2kB ± 0% -99.31% (p=0.000 n=10+7) PlanSuccess_Summary/changes_and_changes_outside-10 14.5kB ± 0% 0.2kB ± 0% -98.78% (p=0.000 n=10+8) name old allocs/op new allocs/op delta PlanSuccess_Summary/empty_summary,_no_matches-10 114 ± 0% 0 -100.00% (p=0.000 n=10+10) PlanSuccess_Summary/changes-10 69.0 ± 0% 0.0 -100.00% (p=0.000 n=10+10) PlanSuccess_Summary/no_changes-10 114 ± 0% 0 -100.00% (p=0.000 n=10+10) PlanSuccess_Summary/changes_outside_Terraform-10 117 ± 0% 3 ± 0% -97.44% (p=0.000 n=10+10) PlanSuccess_Summary/changes_and_changes_outside-10 72.0 ± 0% 3.0 ± 0% -95.83% (p=0.000 n=10+10) Signed-off-by: Leandro López (inkel) * Remove extra allocation There's no need for fmt.Sprintf for such a simple use. Signed-off-by: Leandro López (inkel) * Add MarkdownRenderer benchmark for diff markdown format Signed-off-by: Leandro López (inkel) * Compile markdown diff regexes once This change doesn't improve performance as dramatically as the summary regexes do, however, it does reduce the amount of resources used: $ benchstat markdowndiff.orig.txt markdowndiff.inkel.txt name old time/op new time/op delta RenderProjectResultsWithEnableDiffMarkdownFormat/single_successful_plan_with_diff_markdown_formatted/verbose_true-10 727µs ± 0% 717µs ± 2% -1.31% (p=0.002 n=10+10) RenderProjectResultsWithEnableDiffMarkdownFormat/single_successful_plan_with_diff_markdown_formatted/verbose_false-10 725µs ± 0% 715µs ± 1% -1.33% (p=0.000 n=9+10) name old alloc/op new alloc/op delta RenderProjectResultsWithEnableDiffMarkdownFormat/single_successful_plan_with_diff_markdown_formatted/verbose_true-10 171kB ± 0% 153kB ± 0% -10.60% (p=0.000 n=10+10) RenderProjectResultsWithEnableDiffMarkdownFormat/single_successful_plan_with_diff_markdown_formatted/verbose_false-10 171kB ± 0% 153kB ± 0% -10.60% (p=0.000 n=10+10) name old allocs/op new allocs/op delta RenderProjectResultsWithEnableDiffMarkdownFormat/single_successful_plan_with_diff_markdown_formatted/verbose_true-10 364 ± 0% 214 ± 0% -41.29% (p=0.000 n=10+9) RenderProjectResultsWithEnableDiffMarkdownFormat/single_successful_plan_with_diff_markdown_formatted/verbose_false-10 358 ± 0% 207 ± 0% -42.05% (p=0.000 n=10+10) Signed-off-by: Leandro López (inkel) Signed-off-by: Leandro López (inkel) --- server/events/command/project_result_test.go | 64 ++++++++++++++ server/events/markdown_renderer_test.go | 87 ++++++++++++++------ server/events/models/models.go | 30 ++++--- 3 files changed, 143 insertions(+), 38 deletions(-) diff --git a/server/events/command/project_result_test.go b/server/events/command/project_result_test.go index cd1e7bb67..0c917b9d3 100644 --- a/server/events/command/project_result_test.go +++ b/server/events/command/project_result_test.go @@ -225,3 +225,67 @@ func TestPlanSuccess_Summary(t *testing.T) { }) } } + +var Summary string + +func BenchmarkPlanSuccess_Summary(b *testing.B) { + var s string + + fixtures := map[string]string{ + "changes": ` + An execution plan has been generated and is shown below. + Resource actions are indicated with the following symbols: + - destroy + + Terraform will perform the following actions: + + - null_resource.hi[1] + + + Plan: 0 to add, 0 to change, 1 to destroy.`, + "no changes": ` + An execution plan has been generated and is shown below. + Resource actions are indicated with the following symbols: + + No changes. Infrastructure is up-to-date.`, + "changes outside Terraform": ` + Note: Objects have changed outside of Terraform + + Terraform detected the following changes made outside of Terraform since the + last "terraform apply": + + No changes. Your infrastructure matches the configuration.`, + "changes and changes outside": ` + Note: Objects have changed outside of Terraform + + Terraform detected the following changes made outside of Terraform since the + last "terraform apply": + + An execution plan has been generated and is shown below. + Resource actions are indicated with the following symbols: + - destroy + + Terraform will perform the following actions: + + - null_resource.hi[1] + + + Plan: 0 to add, 0 to change, 1 to destroy.`, + "empty summary, no matches": `No match, expect empty`, + } + + for name, output := range fixtures { + p := &models.PlanSuccess{ + TerraformOutput: output, + } + + b.Run(name, func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + s = p.Summary() + } + + Summary = s + }) + } +} diff --git a/server/events/markdown_renderer_test.go b/server/events/markdown_renderer_test.go index 094ef1c56..6a9151a3c 100644 --- a/server/events/markdown_renderer_test.go +++ b/server/events/markdown_renderer_test.go @@ -2214,8 +2214,7 @@ $$$ } } -func TestRenderProjectResultsWithEnableDiffMarkdownFormat(t *testing.T) { - tfOutput := `An execution plan has been generated and is shown below. +const tfOutput = `An execution plan has been generated and is shown below. Resource actions are indicated with the following symbols: ~ update in-place -/+ destroy and then create replacement @@ -2398,30 +2397,31 @@ Terraform will perform the following actions: Plan: 1 to add, 2 to change, 1 to destroy. ` - cases := []struct { - Description string - Command command.Name - ProjectResults []command.ProjectResult - VCSHost models.VCSHostType - Expected string - }{ - { - "single successful plan with diff markdown formatted", - command.Plan, - []command.ProjectResult{ - { - PlanSuccess: &models.PlanSuccess{ - TerraformOutput: tfOutput, - LockURL: "lock-url", - RePlanCmd: "atlantis plan -d path -w workspace", - ApplyCmd: "atlantis apply -d path -w workspace", - }, - Workspace: "workspace", - RepoRelDir: "path", + +var cases = []struct { + Description string + Command command.Name + ProjectResults []command.ProjectResult + VCSHost models.VCSHostType + Expected string +}{ + { + "single successful plan with diff markdown formatted", + command.Plan, + []command.ProjectResult{ + { + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: tfOutput, + LockURL: "lock-url", + RePlanCmd: "atlantis plan -d path -w workspace", + ApplyCmd: "atlantis apply -d path -w workspace", }, + Workspace: "workspace", + RepoRelDir: "path", }, - models.Github, - `Ran Plan for dir: $path$ workspace: $workspace$ + }, + models.Github, + `Ran Plan for dir: $path$ workspace: $workspace$
Show Output @@ -2619,8 +2619,10 @@ Plan: 1 to add, 2 to change, 1 to destroy. `, - }, - } + }, +} + +func TestRenderProjectResultsWithEnableDiffMarkdownFormat(t *testing.T) { r := events.GetMarkdownRenderer( false, // GitlabSupportsCommonMark true, // DisableApplyAll @@ -2650,3 +2652,36 @@ Plan: 1 to add, 2 to change, 1 to destroy. }) } } + +var Render string + +func BenchmarkRenderProjectResultsWithEnableDiffMarkdownFormat(b *testing.B) { + var render string + + r := events.GetMarkdownRenderer( + false, // GitlabSupportsCommonMark + true, // DisableApplyAll + true, // DisableApply + false, // DisableMarkdownFolding + false, // DisableRepoLocking + true, // EnableDiffMarkdownFormat + "", // MarkdownTemplateOverridesDir + ) + + for _, c := range cases { + b.Run(c.Description, func(b *testing.B) { + res := command.Result{ + ProjectResults: c.ProjectResults, + } + for _, verbose := range []bool{true, false} { + b.Run(fmt.Sprintf("verbose %t", verbose), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + render = r.Render(res, c.Command, "log", verbose, c.VCSHost) + } + Render = render + }) + } + }) + } +} diff --git a/server/events/models/models.go b/server/events/models/models.go index 0ca9e8d94..7ebcca65c 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -366,28 +366,34 @@ type PlanSuccess struct { HasDiverged bool } +// Summary regexes +var ( + reChangesOutside = regexp.MustCompile(`Note: Objects have changed outside of Terraform`) + rePlanChanges = regexp.MustCompile(`Plan: \d+ to add, \d+ to change, \d+ to destroy.`) + reNoChanges = regexp.MustCompile(`No changes. (Infrastructure is up-to-date|Your infrastructure matches the configuration).`) +) + // Summary extracts one line summary of plan changes from TerraformOutput. func (p *PlanSuccess) Summary() string { note := "" - r := regexp.MustCompile(`Note: Objects have changed outside of Terraform`) - if match := r.FindString(p.TerraformOutput); match != "" { - note = fmt.Sprintf("\n**%s**\n", match) + if match := reChangesOutside.FindString(p.TerraformOutput); match != "" { + note = "\n**" + match + "**\n" } - - r = regexp.MustCompile(`Plan: \d+ to add, \d+ to change, \d+ to destroy.`) - if match := r.FindString(p.TerraformOutput); match != "" { + if match := rePlanChanges.FindString(p.TerraformOutput); match != "" { return note + match } - r = regexp.MustCompile(`No changes. (Infrastructure is up-to-date|Your infrastructure matches the configuration).`) - return note + r.FindString(p.TerraformOutput) + return note + reNoChanges.FindString(p.TerraformOutput) } +// Diff Markdown regexes +var ( + diffKeywordRegex = regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\)| {2,}[^ ]+:.*)(.*)`) + diffListRegex = regexp.MustCompile(`(?m)^( +)([-+~]\s)(".*",)`) + diffTildeRegex = regexp.MustCompile(`(?m)^~`) +) + // DiffMarkdownFormattedTerraformOutput formats the Terraform output to match diff markdown format func (p PlanSuccess) DiffMarkdownFormattedTerraformOutput() string { - diffKeywordRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\)| {2,}[^ ]+:.*)(.*)`) - diffListRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(".*",)`) - diffTildeRegex := regexp.MustCompile(`(?m)^~`) - formattedTerraformOutput := diffKeywordRegex.ReplaceAllString(p.TerraformOutput, "$2$1$3$4$5") formattedTerraformOutput = diffListRegex.ReplaceAllString(formattedTerraformOutput, "$2$1$3") formattedTerraformOutput = diffTildeRegex.ReplaceAllString(formattedTerraformOutput, "!")