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

Drop pointer indirection of app in SingleCommandApp and MultiCommandApp #1

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

dolmen
Copy link
Contributor

@dolmen dolmen commented Aug 14, 2023

When embedding app in SingleCommandApp or MultiCommandApp, drop the unnecessary pointer indirection.

@Rican7 Rican7 self-assigned this Aug 14, 2023
@Rican7 Rican7 self-requested a review August 14, 2023 06:06
@Rican7 Rican7 added the enhancement New feature or request label Aug 14, 2023
@Rican7
Copy link
Owner

Rican7 commented Aug 14, 2023

Hmm, this is interesting... I understand that the SingleCommandApp and MultiCommandApp are already returned (and used as) pointers, and their methods contain pointer receivers, so there's less likely a reason to need the app indirection, but what's the benefit of this exactly?

@dolmen
Copy link
Contributor Author

dolmen commented Aug 20, 2023

The benefit is that memory is allocated in a single block and a single call to the allocator.

Do you need a proof with a benchmark?

@Rican7
Copy link
Owner

Rican7 commented Aug 20, 2023

Ooo, I didn't think of that!

I don't need a proof (though it would be really cool for learning), but rather I just wanted to understand the concept before merging.

@Rican7
Copy link
Owner

Rican7 commented Aug 23, 2023

I was curious, so I wrote my own benchmark, and you're right!

I wrote the following simple benchmark in lieut_test.go:

func BenchmarkSingleCommandApp(b *testing.B) {
  for i := 0; i < b.N; i++ {
    flagSet := flag.NewFlagSet(testAppInfo.Name, flag.ExitOnError)
    NewSingleCommandApp(testAppInfo, testNoOpExecutor, flagSet, os.Stdout, os.Stderr)
  }
}

Running go test -bench=. -benchmem shows:

Before Your Changes

goos: linux
goarch: amd64
pkg: github.com/Rican7/lieut
cpu: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
BenchmarkSingleCommandApp-12             1615616               741.9 ns/op           680 B/op         10 allocs/op
PASS
ok      github.com/Rican7/lieut 1.966s

After Your Changes

goos: linux
goarch: amd64
pkg: github.com/Rican7/lieut
cpu: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
BenchmarkSingleCommandApp-12             1787079               730.1 ns/op           680 B/op          9 allocs/op
PASS
ok      github.com/Rican7/lieut 2.000s

@Rican7
Copy link
Owner

Rican7 commented Aug 23, 2023

Ah, this change also seems to "fix" (make consistent?) the different method receivers, as the method promotion of the embedded methods was based on the indirection.

Before

chrome_KCH9NQC08x


After

chrome_01Y0lo3T6Y

Thanks!

@Rican7 Rican7 merged commit 5f7f5dc into Rican7:main Aug 23, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants