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

testing: report allocations in the BenchmarkResult struct Stringer #24296

Closed
titpetric opened this issue Mar 7, 2018 · 8 comments
Closed

testing: report allocations in the BenchmarkResult struct Stringer #24296

titpetric opened this issue Mar 7, 2018 · 8 comments

Comments

@titpetric
Copy link

@titpetric titpetric commented Mar 7, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.10

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Not applicable

What did you do?

Call testing.Benchmark when using b.ReportAllocs() in the benchmark function.

What did you expect to see?

New  1000000          1232 ns/op        0 B/op         0 allocs/op
A.New  1000000        1169 ns/op        0 B/op         0 allocs/op

What did you see instead?

New  1000000          1171 ns/op
A.New  1000000        1115 ns/op
@davecheney

This comment has been minimized.

Copy link
Contributor

@davecheney davecheney commented Mar 7, 2018

This is not the expected behaviour. Can you please provide the code sample that produced the output above.

@titpetric

This comment has been minimized.

Copy link
Author

@titpetric titpetric commented Mar 7, 2018

Sure, here is the playground link, crippled a bit to give you some output (ie, doesn't use b.N).

The sources for the func (r BenchmarkResult) String() string here basically don't give a call to r.MemString(), or have access to B.showAllocResult, because the *B or showAllocResult isn't copied into the struct (ie, there isn't a property for it, yet).

@davecheney

This comment has been minimized.

Copy link
Contributor

@davecheney davecheney commented Mar 7, 2018

@davecheney

This comment has been minimized.

Copy link
Contributor

@davecheney davecheney commented Mar 7, 2018

@titpetric

This comment has been minimized.

Copy link
Author

@titpetric titpetric commented Mar 7, 2018

Yes, feature request. It's an inconvenience with a work-around. Thanks for taking the time to understand it however :)

@andybons andybons changed the title Feature request - report allocations in the BenchmarkResult struct Stringer cmd/go: report allocations in the BenchmarkResult struct Stringer Mar 7, 2018
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Mar 7, 2018

Yes, feature request. It's an inconvenience with a work-around.

Can you outline what your workaround is? I’m having trouble seeing it :)

@titpetric

This comment has been minimized.

Copy link
Author

@titpetric titpetric commented Mar 8, 2018

@andybons it's fmt.Println(result, result.MemString()) instead of just fmt.Println(result).

@ianlancetaylor ianlancetaylor changed the title cmd/go: report allocations in the BenchmarkResult struct Stringer testing: report allocations in the BenchmarkResult struct Stringer Mar 28, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 28, 2018

I don't think we should always print the memory results, and I don't think we should add a new field to BenchmarkResult, a struct that currently has only exported fields. It's already possible to call MemString; adding the results to String would be a minor convenience in a few cases. So all in all I don't think this is worth doing. Sorry.

@golang golang locked and limited conversation to collaborators Mar 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.