Optimise runtime for passing tests

[bboreham]

Summary

Move some calls to testing.Helper() to reduce overhead.

Changes

Move call to Helper() after the check in various straightforward assert cases.

Short-circuit require.NoError() and require.NoErrorf() so they do not call Helper() when no error is found. - deferred till another time

Motivation

Helper() in the standard Go runtime fetches a stack trace from the runtime, so is slow for calls that are made many times. However, Helper() only makes a difference if the call throws an error, so we can avoid that overhead in many cases which are otherwise cheap.

Related issues

#766 (not a full fix)

[boyan-soubachov]

I like the idea behind this PR but there are several things that need to happen.

1. Most importantly, could you please run a benchmark (most likely with testing.TB) to prove that the changes actually reduce the run time of tests. This is also a great way for us to gauge by what factor the speed-up happens
2. It seems you've forgotten to run the go generate ./... function before committing as we get an error in the builds

Go generate had not been run

[bboreham]

run the go generate ./... function

This information is not in CONTRIBUTING.md. Would you like a PR to move it from README.md and add a link?

[boyan-soubachov]

Yea, that makes sense. Thank you

…

On 28 Apr 2020, at 05:29, Bryan Boreham ***@***.***> wrote:  run the go generate ./... function This information is not in CONTRIBUTING.md. Would you like a PR to move it from README.md and add a link? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[bboreham]

I didn't realise require.go was generated; changing require.NoError will take a bit more work.

I have re-pushed without that commit and with a benchmark.

Before (best of 5):

BenchmarkNotNil-2   	 1250132	       919 ns/op	     216 B/op	       2 allocs/op

After:

BenchmarkNotNil-2   	80539458	        14.8 ns/op	       0 B/op	       0 allocs/op

most likely with testing.TB

I didn't get this bit. Could you explain more?

[boyan-soubachov]

I didn't realise require.go was generated; changing require.NoError will take a bit more work.

Almost everything in assert and require is generated, it all gets derived from assertions.go. I don't think you should have any problems with require.NoError(). What kind of issues do you foresee?

I have re-pushed without that commit and with a benchmark.

Before (best of 5):

BenchmarkNotNil-2   	 1250132	       919 ns/op	     216 B/op	       2 allocs/op

After:

BenchmarkNotNil-2   	80539458	        14.8 ns/op	       0 B/op	       0 allocs/op

That looks great!

most likely with testing.TB

I didn't get this bit. Could you explain more?

What I meant is that we could change the unit tests of some (or all if you're so inclined) of the functions you optimised to use testing.TB which allows benchmarks to be run. There's no need to assert that the new method is faster but it does help in being able to easily compare the performance difference.

[bboreham]

What I originally did in require.NoError was add if err==nil { return } at the top. This would require some sort of special-casing in the code generator.

Maybe it would be more fruitful to generate a copy of the assert code rather than call the function, because otherwise we‘re calling t.Helper() twice in many cases.

[boyan-soubachov]

What I originally did in require.NoError was add if err==nil { return } at the top. This would require some sort of special-casing in the code generator.

Yes, but if you re-run go generate ./... with the changes proposed in this PR, does it not move the helper function call in require.NotNil() till after the assert.NotNil() call?

Maybe it would be more fruitful to generate a copy of the assert code rather than call the function, because otherwise we‘re calling t.Helper() twice in many cases.

Agreed but that would be a much bigger change in _codegen that should be done in a separate PR in my opinion.

[bboreham]

No, this PR does not change require.go. The order of calls is in the generator template.

[boyan-soubachov]

Yes, but if I'm not mistaken, running go generate ./... in the repo will change require.go?

[bboreham]

Didn’t happen for me.
The generation is based off the function signature, and I haven’t changed any of those.

Note I did have a problem at the beginning that I had hand-edited require.go, which caused an error saying I needed to re-generate, but I’ve removed that commit from this PR.

[boyan-soubachov]

I see. Something isn’t right then since the auto generation should create the require.go file if I remember correctly.

…

On 29 Apr 2020, at 03:24, Bryan Boreham ***@***.***> wrote:  Didn’t happen for me. The generation is based off the function signature, and I haven’t changed any of those. Note I did have a problem at the beginning that I had hand-edited require.go, which caused an error saying I needed to re-generate, but I’ve removed that commit from this PR. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[bboreham]

Yes the generator creates require.go, and it is identical to the previous checked-in file. Hence it does not change in this PR.

[boyan-soubachov]

Yes the generator creates require.go, and it is identical to the previous checked-in file. Hence it does not change in this PR.

I see. This is obviously a problem then. Doesn't seem right that we change a function in assertions.go and the generate code does not change requires.go.

[boyan-soubachov]

@bboreham , I have found the cause/issue.

In require/require.go.tmpl, we always prepend a helper function type assertion & call. If you remove the line (line 3: if h, ok := t.(tHelper); ok { h.Helper() }) and you rerun go generate it will then remove that helper call for all require.go functions.

This is fine in my opinion since the require.go functions are meant to call out to their equivalent assert functions and return a FailNow() instead of a Fail(), where appropriate.

The only important factor to this is that it will change a large amount of code, so if you're willing to have a thorough look and make sure that everything is being called correctly, I'll be happy to double-check and approve if all is fine :)

[bboreham]

I think that would mean failures are reported inside require.go.
See #650 for further discussion on this point.

[boyan-soubachov]

I think that would mean failures are reported inside require.go.
See #650 for further discussion on this point.

I see, very good point. I missed that.

I'm very happy to merge it in its current format then. I guess optimisation of some parts is better than none :) Thank you for your effort and time!