Skip to content
This repository was archived by the owner on Dec 1, 2021. It is now read-only.

Conversation

@bradleyfalzon
Copy link
Contributor

@bradleyfalzon bradleyfalzon commented Feb 27, 2017

toperr is not used, but the go compiler itself doesn't detect this
because it's within an anonymous function. However, go/types does
detect this as being unused, which causes any static analysis tools
which uses go/types' type checker to fail with the message "toperr
assigned and not used".

The final result of the benchmarked function is instead assigned to
an exported global variable to ensure the compiler cannot now, nor
in the future optimise away the function calls due to no observable
side effects.

It was chosen to assign the final result, after the benchmark loop,
to the global variable, as this best follows the example set in the
CL https://go-review.googlesource.com/#/c/37195/. As opposed to
having each call to f assign to the global. This also appears to
better align with the original author's intention of toperr.

This change had no observable impact on the benchmark.

Related golang/go#3059.
Related golang/go#8560.

Thanks @dominikh for additional clarifications.

Example playground showing Go compiler is OK with unused variables in anonymous functions: https://play.golang.org/p/MEBjbLDWzC but go/types is not: https://play.golang.org/p/m7Gt4tEgS2.

benchstat compare of master and this branch, but I couldn't find a quiet enough machine to better show no change.

name                            old time/op    new time/op    delta
Errors/pkg/errors-stack-10-4      1.40µs ± 0%    1.41µs ± 0%  +0.33%        (p=0.000 n=18+18)
Errors/errors-stack-10-4          53.4ns ± 0%    53.1ns ± 0%  -0.62%        (p=0.000 n=16+16)
Errors/pkg/errors-stack-100-4     2.46µs ± 0%    2.46µs ± 1%  +0.18%        (p=0.027 n=20+20)
Errors/errors-stack-100-4          397ns ± 0%     398ns ± 0%  +0.32%        (p=0.003 n=12+18)
Errors/pkg/errors-stack-1000-4    8.42µs ± 1%    8.46µs ± 0%  +0.41%        (p=0.000 n=18+20)
Errors/errors-stack-1000-4        3.83µs ± 0%    3.83µs ± 0%    ~           (p=0.266 n=19+19)

name                            old alloc/op   new alloc/op   delta
Errors/pkg/errors-stack-10-4        320B ± 0%      320B ± 0%    ~     (all samples are equal)
Errors/errors-stack-10-4           16.0B ± 0%     16.0B ± 0%    ~     (all samples are equal)
Errors/pkg/errors-stack-100-4       320B ± 0%      320B ± 0%    ~     (all samples are equal)
Errors/errors-stack-100-4          16.0B ± 0%     16.0B ± 0%    ~     (all samples are equal)
Errors/pkg/errors-stack-1000-4      320B ± 0%      320B ± 0%    ~     (all samples are equal)
Errors/errors-stack-1000-4         16.0B ± 0%     16.0B ± 0%    ~     (all samples are equal)

name                            old allocs/op  new allocs/op  delta
Errors/pkg/errors-stack-10-4        3.00 ± 0%      3.00 ± 0%    ~     (all samples are equal)
Errors/errors-stack-10-4            1.00 ± 0%      1.00 ± 0%    ~     (all samples are equal)
Errors/pkg/errors-stack-100-4       3.00 ± 0%      3.00 ± 0%    ~     (all samples are equal)
Errors/errors-stack-100-4           1.00 ± 0%      1.00 ± 0%    ~     (all samples are equal)
Errors/pkg/errors-stack-1000-4      3.00 ± 0%      3.00 ± 0%    ~     (all samples are equal)
Errors/errors-stack-1000-4          1.00 ± 0%      1.00 ± 0%    ~     (all samples are equal)

/cc @kardianos this was originally added in 1d2e603 so you're probably best to review if you have time.

toperr is not used, but the go compiler itself doesn't detect this
because it's within an anonymous function. However, go/types does
detect this as being unused, which causes any static analysis tools
which uses go/types' type checker to fail with the message "toperr
assigned and not used".

The final result of the benchmarked function is instead assigned to
an exported global variable to ensure the compiler cannot now, nor
in the future optimise away the function calls due to no observable
side effects.

It was chosen to assign the final result, after the benchmark loop,
to the global variable, as this best follows the example set in the
CL https://go-review.googlesource.com/#/c/37195/. As opposed to
having each call to f assign to the global. This also appears to
better align with the original author's intention of toperr.

This change had no observable impact on the benchmark.

Related golang/go#3059.
Related golang/go#8560.

Thanks dominikh for additional clarifications.
@kardianos
Copy link
Contributor

LGTM

@davecheney davecheney merged commit bfd5150 into pkg:master Feb 27, 2017
@bradleyfalzon bradleyfalzon deleted the types branch June 3, 2017 03:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants