Conversation
This is an alternative to PRs frankban#160 and frankban#165. It's essentially the same as PR frankban#165 except that it uses generics to reduce the amount of duplicated code. Instead of just amortizing the checking of the type, when the argument type of the function passed to `Run` is known, it bypasses the reflect-based code altogether. We don't bother implementing the optimization on pre-generics Go versions because those are end-of-lifetime anyway. I've added an implementation-independent benchmark. ``` goos: linux goarch: amd64 pkg: github.com/frankban/quicktest cpu: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz │ base │ thisPR │ │ sec/op │ sec/op vs base │ CNewAndRunWithCustomType-8 1077.5n ± 5% 136.8n ± 6% -87.30% (p=0.002 n=6) CRunWithCustomType-8 1035.00n ± 11% 66.43n ± 3% -93.58% (p=0.002 n=6) geomean 1.056µ 95.33n -90.97% ```
rogpeppe
force-pushed
the
021-faster-run
branch
from
fecf9c7 to
86c74a3
Compare
| case runner[*testing.B]: | ||
| return fastRun1(c, name, f, t), true | ||
| case runner[*C]: | ||
| return fastRun1(c, name, f, t), true |
There was a problem hiding this comment.
Should we return fastRun(t, name, f) here?
There was a problem hiding this comment.
It could be possible as a shortcut, but maybe a bit too clever. In fact this shortcut assumes that t is if of type *qt.C which may not be the case (a non-qt.C might have a Run(string, *qt.C) bool method).
Another alternative which is less clever, but still shortcuts (avoids to create one qt.C) while not corrupting the interface:
| return fastRun1(c, name, f, t), true | |
| return t.Run(name, f), true |
Note that this shortcut is already implemented in #165.
There was a problem hiding this comment.
FWIW I thought about this, and even implemented it, but decided it wasn't a good idea. For one, it only saves one allocation, which is a fairly minor saving in this context. For another, it means that it's not consistent with the other types, which always allocate a new C. Making this optimisation changes observable behaviour and because I'm not convinced it's a worthwhile optimisation anyway, I'd be inclined to leave it out.
There was a problem hiding this comment.
Good catch.
One observable difference would be the format function that should be propagated from the parent C. We obviously lack a test that would have caught this.
| t.Errorf("TB isn't expected type (want %v got %T)", test.rtype(), c.TB) | ||
| } | ||
| }) | ||
| if got, want := called, 1; got != want { |
There was a problem hiding this comment.
Also maybe s/called/callCount/, as called feels like a boolean.
| return false, false | ||
| } | ||
|
|
||
| type runner[T any] interface { |
There was a problem hiding this comment.
This is a very nice way of not exposing generics in the public API!
dolmen
left a comment
dolmen
left a comment
There was a problem hiding this comment.
Generics are used here just to avoid duplicating the 5 lines of code in fastRun1.
This brings no performance benefits over #165 (in fact Go versions below 1.18 still get the reflect version which is not the case in #165), but makes the code much more messy. Because with generics come build tags to maintain.
The use of generics here is just over engineering.
|
|
||
| func (customTForBenchmark) Run(name string, f func(testing.TB)) bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The benchmarks could be moved to a separate PR as they would be useful for the other alternatives.
|
The commit message claims:
Well. To remove 7 lines which, in #165, are duplicated just once 10 lines below (and not exact duplicate as this allows to call Is it really worth it? I don't think so. This is just a bad use case for generics. Note: CI is failing on Go 1.13. |
3 participants
This is an alternative to PRs #160 and #165.
It's essentially the same as PR #165 except that it uses generics to reduce the amount of duplicated code.
Instead of just amortizing the checking of the type, when the argument type of the function passed to
Runis known, it bypasses the reflect-based code altogether.We don't bother implementing the optimization on pre-generics Go versions because those are end-of-lifetime anyway.
I've added an implementation-independent benchmark.