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.

Sign up for GitHub

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

Jump to bottom

small optimization for fsfinder #136

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

small optimization for fsfinder #136

wants to merge 12 commits into from

Conversation

SkobelkinYaroslav
Copy link
Contributor

some optimization and added benchmark to test it

@kehoecj kehoecj added the OSS Community Contribution Contributions from the OSS Community label Apr 4, 2024
@kehoecj kehoecj self-requested a review April 10, 2024 15:23
@kehoecj kehoecj added the waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers label Apr 11, 2024
@kehoecj
Copy link
Contributor

kehoecj commented May 30, 2024 •
edited

Output of the benchmark run go test -v --bench=Benchmark_Finder -benchtime 5s -benchmem ./...

goos: darwin
goarch: amd64
pkg: github.com/Boeing/config-file-validator/pkg/finder
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
Benchmark_Finder
Benchmark_Finder-12    	    6346	    823069 ns/op	   75869 B/op	     628 allocs/op

and here's the benchmark against the main branch finder:

goos: darwin
goarch: amd64
pkg: github.com/Boeing/config-file-validator/pkg/finder
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
Benchmark_Finder
Benchmark_Finder-12    	    7038	    769833 ns/op	   79535 B/op	     634 allocs/op

Some pretty clear improvements. Thanks @SkobelkinYaroslav !

kehoecj
kehoecj approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

LGTM! - one comment on commented code

pkg/finder/finder_test.go
@@ -168,9 +168,9 @@ func Test_FileSystemFinderMultipleFinder(t *testing.T) {
func Test_FileSystemFinderDuplicateFiles(t *testing.T) {
fsFinder := FileSystemFinderInit(
WithPathRoots(
"../../test/fixtures/subdir/good.json",
//"../../test/fixtures/subdir/good.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend removing the commented code if it is no longer needed

@kehoecj kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kehoecj kehoecj kehoecj approved these changes

Assignees
No one assigned
Labels
OSS Community Contribution Contributions from the OSS Community pr-action-requested PR is awaiting feedback from the submitting developer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@SkobelkinYaroslav @kehoecj