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

Fix: Improve performance of content destination collision detection. #388

Merged

Conversation

erikgeiser
Copy link
Member

@erikgeiser erikgeiser commented Oct 23, 2021

This PR fixes #387 and #393 by reducing the algorithmic complexity of the content destination collision detection. Previously when appending an element, the destination was compared to all other elements in the content list. Now we simply append and check for collisions once at the end with a single pass through all elements.

For 150000 files with random destinations, the performance compares as follows:

Before: BenchmarkExpandContentGlobs-4   	       1	2982879539110 ns/op
Now   : BenchmarkExpandContentGlobs-4   	       1	9052880061 ns/op

That's a 320x improvement from 50 minutes to 9 seconds for extreme use cases!

Here's the benchmark code:

func BenchmarkExpandContentGlobs(b *testing.B) {
	contents := testContents(150000)
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		_, err := files.ExpandContentGlobs(contents, true)
		if err != nil {
			b.Fatalf("expand content globs: %v", err)
		}
	}
}

func testContents(n int) files.Contents {
	contents := make(files.Contents, n)

	for i := 0; i < n; i++ {
		contents[i] = &files.Content{
			Source:      "../testdata/whatever.conf",
			Destination: randomString(),
		}
	}

	return contents
}

const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"

func randomString() string {
	b := make([]byte, 5+rand.Intn(20))
	for i := range b {
		b[i] = letters[rand.Intn(len(letters))]
	}
	return string(b)
}

Thank you @mjmjelde for reporting this.

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 23, 2021
@vercel vercel bot temporarily deployed to Preview October 23, 2021 18:46 Inactive
@codecov
Copy link

codecov bot commented Oct 23, 2021

Codecov Report

Merging #388 (b29e3c8) into master (685352f) will increase coverage by 0.00%.
The diff coverage is 86.36%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #388   +/-   ##
=======================================
  Coverage   64.66%   64.67%           
=======================================
  Files          14       14           
  Lines        1834     1826    -8     
=======================================
- Hits         1186     1181    -5     
+ Misses        512      509    -3     
  Partials      136      136           
Impacted Files Coverage Δ
internal/cmd/package.go 0.00% <ø> (ø)
files/files.go 95.45% <85.71%> (-0.20%) ⬇️
nfpm.go 86.36% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 685352f...b29e3c8. Read the comment docs.

Copy link
Contributor

@djgilcrease djgilcrease left a comment

Choose a reason for hiding this comment

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

LGTM

@vercel vercel bot temporarily deployed to Preview November 4, 2021 19:50 Inactive
@vercel vercel bot temporarily deployed to Preview November 5, 2021 20:22 Inactive
@erikgeiser
Copy link
Member Author

I added a commit that removes the duplicate Validate call to fix #393.

@vercel vercel bot temporarily deployed to Preview November 5, 2021 21:12 Inactive
@vercel vercel bot temporarily deployed to Preview November 5, 2021 22:00 Inactive
@caarlos0 caarlos0 merged commit 3f068f0 into goreleaser:master Nov 6, 2021
@caarlos0
Copy link
Member

caarlos0 commented Nov 6, 2021

amazing! thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance of content collision detection
3 participants