[golang-dev] Re: code review 6819087: test/bench/go: new package (issue 6819087)

LGTM



https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go
File test/bench/go/asop_test.go (right):

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go#newcode12
test/bench/go/asop_test.go:12: }
I would suggest add a global

var x int

and then after each of these loops insert a use of the variable, like:

x += v

just so that a smarter compiler does not (1) complain that v is never
really used and (2) does not throw the entire loop away.

Thanks.

https://codereview.appspot.com/6819087/

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go
File test/bench/go/asop_test.go (right):

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go#newcode9
test/bench/go/asop_test.go:9: func BenchmarkLSL(b *testing.B) {
it seems if you have these, you might as well finish the list.

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/doc.go
File test/bench/go/doc.go (right):

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/doc.go#newcode5
test/bench/go/doc.go:5: // Package bench contains low level code
generation micro benchmarks.
s/micro /micro-/

https://codereview.appspot.com/6819087/

Thank you for your comments. I know these benchmarks must appear
trivial, but they expose some codegen improvements that 5g can make in
the future.


https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go
File test/bench/go/asop_test.go (right):

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go#newcode9
test/bench/go/asop_test.go:9: func BenchmarkLSL(b *testing.B) {
Done.

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go#newcode12
test/bench/go/asop_test.go:12: }
Done.

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/doc.go
File test/bench/go/doc.go (right):

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/doc.go#newcode5
test/bench/go/doc.go:5: // Package bench contains low level code
generation micro benchmarks.
Done.

https://codereview.appspot.com/6819087/

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go#newcode9

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/asop_test.go#newcode12
like:
never really

https://codereview.appspot.com/6819087/diff/10001/test/bench/go/doc.go#newcode5
After consideration I don't think this benchmark needs to live inside
the tree. I will abandon this proposal and move these benchmarks to a
3rd party package.

http://codereview.appspot.com/6819087/

I think they do belong.

-rob

https://codereview.appspot.com/6819087/diff/5005/test/bench/go/divmul_test.go
File test/bench/go/divmul_test.go (right):

https://codereview.appspot.com/6819087/diff/5005/test/bench/go/divmul_test.go#newcode158
test/bench/go/divmul_test.go:158: func BenchmarkMul4(b *testing.B) {
let's be pedantic and name these more accurately

BenchmarkIntMul4 etc.

we can add Float64Mul4 later if we want.

https://codereview.appspot.com/6819087/

Yes, I also realised I need to add benchmarks for int8,uint8,int16,uint16,etc.

Thank you for your encouragement, I'll take another pass when I get home.

It might make sense to generate the benchmarks rather than type them all in.

I use M4 for things like this...
Michael T. Jones | Chief Technology Advocate | mtj@google.com | +1
650-335-5765

I use Go for things like this.

-rob

no doubt.

https://codereview.appspot.com/6819087/diff/5005/test/bench/go/divmul_test.go
File test/bench/go/divmul_test.go (right):

https://codereview.appspot.com/6819087/diff/5005/test/bench/go/divmul_test.go#newcode176
test/bench/go/divmul_test.go:176: func BenchmarkMagicMul10(b *testing.B)
{
if v == 5, the result of this is 42, not 50.

https://codereview.appspot.com/6819087/

Dave, are you going to finalize these microbenchmarks? I don't
understand why the CL is closed.

http://codereview.appspot.com/6819087/

As I've been working on this CL I've learnt that the task is larger then I originally thought so I'm going to try again with a fresh proposal, soon, I promise.