Improve plan summary performance (#2907)

* 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) <[email protected]> * 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) <[email protected]> * Remove extra allocation There's no need for fmt.Sprintf for such a simple use. Signed-off-by: Leandro López (inkel) <[email protected]> * Add MarkdownRenderer benchmark for diff markdown format Signed-off-by: Leandro López (inkel) <[email protected]> * 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) <[email protected]> Signed-off-by: Leandro López (inkel) <[email protected]>
runatlantis · Jan 4, 2023 · 04f1d06 · 04f1d06
1 parent 5bcecf3
commit 04f1d06
Show file tree

Hide file tree

Showing 3 changed files with 143 additions and 38 deletions.
diff --git 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
@@ -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$
 
 <details><summary>Show Output</summary>
 
@@ -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
@@ -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, "!")