From cce487201e4b440a1edb07baa95114be058947b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Mengu=C3=A9?= Date: Thu, 8 Jun 2023 20:17:12 +0200 Subject: [PATCH 1/5] Refactor Run to extract Run signature check Extract signature check of the Run method. This will allow to cache the result in future commits. --- quicktest.go | 62 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/quicktest.go b/quicktest.go index 65243cf..dfb7268 100644 --- a/quicktest.go +++ b/quicktest.go @@ -217,6 +217,38 @@ var ( tbType = reflect.TypeOf(new(testing.TB)).Elem() ) +// getRunFuncSignature checks the signature of the Run method of the type (ex: *testing.T) +// and returns the signature of its function argument (func(t *T) for *testing.T). +func getRunFuncSignature(t reflect.Type) (reflect.Type, error) { + badType := func(detail string) (reflect.Type, error) { + return nil, fmt.Errorf("cannot execute Run with underlying concrete type %s (%s)", t, detail) + } + m, ok := t.MethodByName("Run") + if !ok { + // c.TB doesn't implement a Run method. + return badType("no Run method") + } + mt := m.Type + // fmt.Println(mt) + if mt.NumIn() != 3 || + mt.In(1) != stringType || + mt.NumOut() != 1 || + mt.Out(0) != boolType { + // The Run method doesn't have the right argument counts and types. + return badType("wrong argument count for Run method") + } + farg := mt.In(2) + if farg.Kind() != reflect.Func || + farg.NumIn() != 1 || + farg.NumOut() != 0 || + !farg.In(0).AssignableTo(tbType) { + // The first argument to the Run function arg isn't right. + return badType("bad first argument type for Run method") + } + + return farg, nil +} + // Run runs f as a subtest of t called name. It's a wrapper around // the Run method of c.TB that provides the quicktest checker to f. When // the function completes, c.Done will be called to run any @@ -243,30 +275,12 @@ var ( // A panic is raised when Run is called and the embedded concrete type does not // implement a Run method with a correct signature. func (c *C) Run(name string, f func(c *C)) bool { - badType := func(m string) { - panic(fmt.Sprintf("cannot execute Run with underlying concrete type %T (%s)", c.TB, m)) - } - m := reflect.ValueOf(c.TB).MethodByName("Run") - if !m.IsValid() { - // c.TB doesn't implement a Run method. - badType("no Run method") - } - mt := m.Type() - if mt.NumIn() != 2 || - mt.In(0) != stringType || - mt.NumOut() != 1 || - mt.Out(0) != boolType { - // The Run method doesn't have the right argument counts and types. - badType("wrong argument count for Run method") - } - farg := mt.In(1) - if farg.Kind() != reflect.Func || - farg.NumIn() != 1 || - farg.NumOut() != 0 || - !farg.In(0).AssignableTo(tbType) { - // The first argument to the Run function arg isn't right. - badType("bad first argument type for Run method") + + farg, err := getRunFuncSignature(reflect.TypeOf(c.TB)) + if err != nil { + panic(err.Error()) } + cFormat := c.getFormat() fv := reflect.MakeFunc(farg, func(args []reflect.Value) []reflect.Value { c2 := New(args[0].Interface().(testing.TB)) @@ -275,6 +289,8 @@ func (c *C) Run(name string, f func(c *C)) bool { f(c2) return nil }) + + m := reflect.ValueOf(c.TB).MethodByName("Run") return m.Call([]reflect.Value{reflect.ValueOf(name), fv})[0].Interface().(bool) } From de23bb88bf5a359f0773d91fab01083ca53a51a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Mengu=C3=A9?= Date: Thu, 8 Jun 2023 22:25:48 +0200 Subject: [PATCH 2/5] Add unit test for getRunFuncSignature --- export_test.go | 2 ++ quicktest_test.go | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/export_test.go b/export_test.go index 06b17b7..e54aa92 100644 --- a/export_test.go +++ b/export_test.go @@ -5,4 +5,6 @@ package quicktest var ( Prefixf = prefixf TestingVerbose = &testingVerbose + + GetRunFuncSignature = getRunFuncSignature ) diff --git a/quicktest_test.go b/quicktest_test.go index ec23afb..90cbe7b 100644 --- a/quicktest_test.go +++ b/quicktest_test.go @@ -6,6 +6,7 @@ import ( "bytes" "errors" "fmt" + "reflect" "strings" "testing" @@ -527,6 +528,25 @@ want: `) } +// TestCRunGetFuncSig checks the internal function getRunFuncSignature which is used in Run. +func TestCRunGetFuncSig(t *testing.T) { + for _, tb := range []testing.TB{ + t, // *testing.T + (*testing.B)(nil), // *testing.B + qt.New(t), // *quicktest.C + } { + tbt := reflect.TypeOf(tb) + farg, err := qt.GetRunFuncSignature(tbt) + if err != nil { + t.Error(err) + continue + } + t.Logf("(%s).Run: func(string, %s) bool", tbt, farg) + // useful to create a cache + t.Logf("{T: reflect.TypeOf((%s)(nil)), FArg: reflect.TypeOf((%s)(nil))}", tbt, farg) + } +} + func TestHelper(t *testing.T) { tt := &testingT{} qt.Assert(tt, true, qt.IsFalse) From 7399d60a2bb87e82a71f4a72783e8741d7b5ef6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Mengu=C3=A9?= Date: Thu, 8 Jun 2023 22:50:22 +0200 Subject: [PATCH 3/5] Add benchmark for getRunFuncSignature Add a dummy implementation getRunFuncSignature (it will be replaced by concrete implementation in future commits) called getRunFuncSignature. Add a benchmark that allows to compare the performance of getRunFuncSignatureCache with getRunFuncSignature. Of course the results are currently useless. --- export_test.go | 3 ++- quicktest.go | 5 +++++ quicktest_test.go | 22 ++++++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/export_test.go b/export_test.go index e54aa92..9929ffb 100644 --- a/export_test.go +++ b/export_test.go @@ -6,5 +6,6 @@ var ( Prefixf = prefixf TestingVerbose = &testingVerbose - GetRunFuncSignature = getRunFuncSignature + GetRunFuncSignature = getRunFuncSignature + GetRunFuncSignatureCache = getRunFuncSignatureCache ) diff --git a/quicktest.go b/quicktest.go index dfb7268..da47f08 100644 --- a/quicktest.go +++ b/quicktest.go @@ -249,6 +249,11 @@ func getRunFuncSignature(t reflect.Type) (reflect.Type, error) { return farg, nil } +func getRunFuncSignatureCache(t reflect.Type) (reflect.Type, error) { + // dummy implementation for now + return getRunFuncSignature(t) +} + // Run runs f as a subtest of t called name. It's a wrapper around // the Run method of c.TB that provides the quicktest checker to f. When // the function completes, c.Done will be called to run any diff --git a/quicktest_test.go b/quicktest_test.go index 90cbe7b..7cd3e23 100644 --- a/quicktest_test.go +++ b/quicktest_test.go @@ -547,6 +547,28 @@ func TestCRunGetFuncSig(t *testing.T) { } } +func BenchmarkCRunGetFuncSig(b *testing.B) { + for _, tb := range []testing.TB{ + (*testing.T)(nil), // *testing.T + b, // *testing.B + qt.New(b), // *quicktest.C + } { + tbt := reflect.TypeOf(tb) + + b.Run(tbt.String()+"-no-cache", func(b *testing.B) { + for i := 0; i < b.N; i++ { + _, _ = qt.GetRunFuncSignature(tbt) + } + }) + + b.Run(tbt.String()+"-with-cache", func(b *testing.B) { + for i := 0; i < b.N; i++ { + _, _ = qt.GetRunFuncSignatureCache(tbt) + } + }) + } +} + func TestHelper(t *testing.T) { tt := &testingT{} qt.Assert(tt, true, qt.IsFalse) From d80fe1d1c52c31fbc59c747fc5ce16460470c4d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Mengu=C3=A9?= Date: Fri, 9 Jun 2023 00:28:30 +0200 Subject: [PATCH 4/5] Implement a prefilled cache around getRunFuncSignature This will give a real boost to C.Run by removing most uses of reflect on each Run. Benchmark results: go test -bench BenchmarkCRunGetFuncSig -benchmem BenchmarkCRunGetFuncSig/*testing.T-no-cache-4 562579 2142 ns/op 168 B/op 4 allocs/op BenchmarkCRunGetFuncSig/*testing.T-with-cache-4 183638598 7.514 ns/op 0 B/op 0 allocs/op BenchmarkCRunGetFuncSig/*testing.B-no-cache-4 608715 2626 ns/op 168 B/op 4 allocs/op BenchmarkCRunGetFuncSig/*testing.B-with-cache-4 100000000 11.00 ns/op 0 B/op 0 allocs/op BenchmarkCRunGetFuncSig/*quicktest.C-no-cache-4 636226 2523 ns/op 168 B/op 4 allocs/op BenchmarkCRunGetFuncSig/*quicktest.C-with-cache-4 68991796 48.91 ns/op 0 B/op 0 allocs/op --- quicktest.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/quicktest.go b/quicktest.go index da47f08..a253c80 100644 --- a/quicktest.go +++ b/quicktest.go @@ -249,8 +249,25 @@ func getRunFuncSignature(t reflect.Type) (reflect.Type, error) { return farg, nil } +// Cache of getRunFuncSignature results used by getRunFuncSignatureCache. +var runFuncSigCache = []struct { + T reflect.Type + FArg reflect.Type +}{ + // Prefill with common types + // Use TestCRunGetFuncSig to generate values + {T: reflect.TypeOf((*testing.T)(nil)), FArg: reflect.TypeOf((func(*testing.T))(nil))}, + {T: reflect.TypeOf((*testing.B)(nil)), FArg: reflect.TypeOf((func(*testing.B))(nil))}, + {T: reflect.TypeOf((*C)(nil)), FArg: reflect.TypeOf((func(*C))(nil))}, +} + func getRunFuncSignatureCache(t reflect.Type) (reflect.Type, error) { - // dummy implementation for now + for i := 0; i < len(runFuncSigCache); i++ { + if t == runFuncSigCache[i].T { + return runFuncSigCache[i].FArg, nil + } + } + return getRunFuncSignature(t) } From eb949779ef02304f56ccbfec810263611c0d865a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Mengu=C3=A9?= Date: Fri, 9 Jun 2023 00:30:47 +0200 Subject: [PATCH 5/5] C.Run: enable the cache for checking the Run signature --- quicktest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quicktest.go b/quicktest.go index a253c80..3824af3 100644 --- a/quicktest.go +++ b/quicktest.go @@ -298,7 +298,7 @@ func getRunFuncSignatureCache(t reflect.Type) (reflect.Type, error) { // implement a Run method with a correct signature. func (c *C) Run(name string, f func(c *C)) bool { - farg, err := getRunFuncSignature(reflect.TypeOf(c.TB)) + farg, err := getRunFuncSignatureCache(reflect.TypeOf(c.TB)) if err != nil { panic(err.Error()) }