From 77a2113925b516c0ead2ae258c4d41ac3fdc0836 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Tue, 23 Dec 2014 18:28:02 -0800 Subject: [PATCH] cmd/gc: evaluate concrete == interface without allocating Consider an interface value i of type I and concrete value c of type C. Prior to this CL, i==c was evaluated as I(c) == i Evaluating I(c) can allocate. This CL changes the evaluation of i==c to x, ok := i.(C); ok && x == c The new generated code is shorter and does not allocate directly. If C is small, as it is in every instance in the stdlib, the new code also uses less stack space and makes one runtime call instead of two. If C is very large, the original implementation is used. The cutoff for "very large" is 1<<16, following the stack vs heap cutoff used elsewhere. This kind of comparison occurs in 38 places in the stdlib, mostly in the net and os packages. benchmark old ns/op new ns/op delta BenchmarkEqEfaceConcrete 29.5 7.92 -73.15% BenchmarkEqIfaceConcrete 32.1 7.90 -75.39% BenchmarkNeEfaceConcrete 29.9 7.90 -73.58% BenchmarkNeIfaceConcrete 35.9 7.90 -77.99% Fixes #9370. Change-Id: I7c4555950bcd6406ee5c613be1f2128da2c9a2b7 Reviewed-on: https://go-review.googlesource.com/2096 Reviewed-by: Russ Cox Run-TryBot: Josh Bleecher Snyder --- src/cmd/gc/typecheck.c | 36 ++++++---- src/cmd/gc/walk.c | 41 ++++++++++++ src/runtime/iface_test.go | 42 ++++++++++++ test/fixedbugs/issue9370.go | 127 ++++++++++++++++++++++++++++++++++++ test/live.go | 5 +- 5 files changed, 234 insertions(+), 17 deletions(-) create mode 100644 test/fixedbugs/issue9370.go diff --git a/src/cmd/gc/typecheck.c b/src/cmd/gc/typecheck.c index 635d2c4170a..64b80a88cd1 100644 --- a/src/cmd/gc/typecheck.c +++ b/src/cmd/gc/typecheck.c @@ -570,6 +570,7 @@ reswitch: et = t->etype; if(et == TIDEAL) et = TINT; + aop = 0; if(iscmp[n->op] && t->etype != TIDEAL && !eqtype(l->type, r->type)) { // comparison is okay as long as one side is // assignable to the other. convert so they have @@ -577,16 +578,20 @@ reswitch: // // the only conversion that isn't a no-op is concrete == interface. // in that case, check comparability of the concrete type. + // The conversion allocates, so only do it if the concrete type is huge. if(r->type->etype != TBLANK && (aop = assignop(l->type, r->type, nil)) != 0) { if(isinter(r->type) && !isinter(l->type) && algtype1(l->type, nil) == ANOEQ) { yyerror("invalid operation: %N (operator %O not defined on %s)", n, op, typekind(l->type)); goto error; } - l = nod(aop, l, N); - l->type = r->type; - l->typecheck = 1; - n->left = l; - t = l->type; + dowidth(l->type); + if(isinter(r->type) == isinter(l->type) || l->type->width >= 1<<16) { + l = nod(aop, l, N); + l->type = r->type; + l->typecheck = 1; + n->left = l; + } + t = r->type; goto converted; } if(l->type->etype != TBLANK && (aop = assignop(r->type, l->type, nil)) != 0) { @@ -594,11 +599,14 @@ reswitch: yyerror("invalid operation: %N (operator %O not defined on %s)", n, op, typekind(r->type)); goto error; } - r = nod(aop, r, N); - r->type = l->type; - r->typecheck = 1; - n->right = r; - t = r->type; + dowidth(r->type); + if(isinter(r->type) == isinter(l->type) || r->type->width >= 1<<16) { + r = nod(aop, r, N); + r->type = l->type; + r->typecheck = 1; + n->right = r; + } + t = l->type; } converted: et = t->etype; @@ -609,8 +617,10 @@ reswitch: yyerror("invalid operation: %N (non-numeric type %T)", n, l->type); goto error; } - yyerror("invalid operation: %N (mismatched types %T and %T)", n, l->type, r->type); - goto error; + if(isinter(r->type) == isinter(l->type) || aop == 0) { + yyerror("invalid operation: %N (mismatched types %T and %T)", n, l->type, r->type); + goto error; + } } if(!okfor[op][et]) { yyerror("invalid operation: %N (operator %O not defined on %s)", n, op, typekind(t)); @@ -685,7 +695,7 @@ reswitch: n->right = l; } else if(r->op == OLITERAL && r->val.ctype == CTNIL) { // leave alone for back end - } else { + } else if(isinter(r->type) == isinter(l->type)) { n->etype = n->op; n->op = OCMPIFACE; } diff --git a/src/cmd/gc/walk.c b/src/cmd/gc/walk.c index e2d74e46bc9..aed5e33a60f 100644 --- a/src/cmd/gc/walk.c +++ b/src/cmd/gc/walk.c @@ -3339,10 +3339,51 @@ static void walkcompare(Node **np, NodeList **init) { Node *n, *l, *r, *call, *a, *li, *ri, *expr, *cmpl, *cmpr; + Node *x, *ok; int andor, i, needsize; Type *t, *t1; n = *np; + + // Given interface value l and concrete value r, rewrite + // l == r + // to + // x, ok := l.(type(r)); ok && x == r + // Handle != similarly. + // This avoids the allocation that would be required + // to convert r to l for comparison. + l = N; + r = N; + if(isinter(n->left->type) && !isinter(n->right->type)) { + l = n->left; + r = n->right; + } else if(!isinter(n->left->type) && isinter(n->right->type)) { + l = n->right; + r = n->left; + } + if(l != N) { + x = temp(r->type); + ok = temp(types[TBOOL]); + + // l.(type(r)) + a = nod(ODOTTYPE, l, N); + a->type = r->type; + + // x, ok := l.(type(r)) + expr = nod(OAS2, N, N); + expr->list = list1(x); + expr->list = list(expr->list, ok); + expr->rlist = list1(a); + typecheck(&expr, Etop); + walkexpr(&expr, init); + + if(n->op == OEQ) + r = nod(OANDAND, ok, nod(OEQ, x, r)); + else + r = nod(OOROR, nod(ONOT, ok, N), nod(ONE, x, r)); + *init = list(*init, expr); + goto ret; + } // Must be comparison of array or struct. // Otherwise back end handles it. diff --git a/src/runtime/iface_test.go b/src/runtime/iface_test.go index bca0ea0ee75..bfeb94b8aa7 100644 --- a/src/runtime/iface_test.go +++ b/src/runtime/iface_test.go @@ -5,6 +5,7 @@ package runtime_test import ( + "runtime" "testing" ) @@ -38,6 +39,47 @@ var ( tl TL ) +// Issue 9370 +func TestCmpIfaceConcreteAlloc(t *testing.T) { + if runtime.Compiler != "gc" { + t.Skip("skipping on non-gc compiler") + } + + n := testing.AllocsPerRun(1, func() { + _ = e == ts + _ = i1 == ts + _ = e == 1 + }) + + if n > 0 { + t.Fatalf("iface cmp allocs=%v; want 0", n) + } +} + +func BenchmarkEqEfaceConcrete(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = e == ts + } +} + +func BenchmarkEqIfaceConcrete(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = i1 == ts + } +} + +func BenchmarkNeEfaceConcrete(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = e != ts + } +} + +func BenchmarkNeIfaceConcrete(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = i1 != ts + } +} + func BenchmarkConvT2ESmall(b *testing.B) { for i := 0; i < b.N; i++ { e = ts diff --git a/test/fixedbugs/issue9370.go b/test/fixedbugs/issue9370.go new file mode 100644 index 00000000000..120af353976 --- /dev/null +++ b/test/fixedbugs/issue9370.go @@ -0,0 +1,127 @@ +// errorcheck + +// Copyright 2014 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Verify that concrete/interface comparisons are +// typechecked correctly by the compiler. + +package main + +type I interface { + Method() +} + +type C int + +func (C) Method() {} + +type G func() + +func (G) Method() {} + +var ( + e interface{} + i I + c C + n int + f func() + g G +) + +var ( + _ = e == c + _ = e != c + _ = e >= c // ERROR "invalid operation.*not defined" + _ = c == e + _ = c != e + _ = c >= e // ERROR "invalid operation.*not defined" + + _ = i == c + _ = i != c + _ = i >= c // ERROR "invalid operation.*not defined" + _ = c == i + _ = c != i + _ = c >= i // ERROR "invalid operation.*not defined" + + _ = e == n + _ = e != n + _ = e >= n // ERROR "invalid operation.*not defined" + _ = n == e + _ = n != e + _ = n >= e // ERROR "invalid operation.*not defined" + + // i and n are not assignable to each other + _ = i == n // ERROR "invalid operation.*mismatched types" + _ = i != n // ERROR "invalid operation.*mismatched types" + _ = i >= n // ERROR "invalid operation.*mismatched types" + _ = n == i // ERROR "invalid operation.*mismatched types" + _ = n != i // ERROR "invalid operation.*mismatched types" + _ = n >= i // ERROR "invalid operation.*mismatched types" + + _ = e == 1 + _ = e != 1 + _ = e >= 1 // ERROR "invalid operation.*not defined" + _ = 1 == e + _ = 1 != e + _ = 1 >= e // ERROR "invalid operation.*not defined" + + _ = i == 1 // ERROR "invalid operation.*mismatched types" + _ = i != 1 // ERROR "invalid operation.*mismatched types" + _ = i >= 1 // ERROR "invalid operation.*mismatched types" + _ = 1 == i // ERROR "invalid operation.*mismatched types" + _ = 1 != i // ERROR "invalid operation.*mismatched types" + _ = 1 >= i // ERROR "invalid operation.*mismatched types" + + _ = e == f // ERROR "invalid operation.*not defined" + _ = e != f // ERROR "invalid operation.*not defined" + _ = e >= f // ERROR "invalid operation.*not defined" + _ = f == e // ERROR "invalid operation.*not defined" + _ = f != e // ERROR "invalid operation.*not defined" + _ = f >= e // ERROR "invalid operation.*not defined" + + _ = i == f // ERROR "invalid operation.*mismatched types" + _ = i != f // ERROR "invalid operation.*mismatched types" + _ = i >= f // ERROR "invalid operation.*mismatched types" + _ = f == i // ERROR "invalid operation.*mismatched types" + _ = f != i // ERROR "invalid operation.*mismatched types" + _ = f >= i // ERROR "invalid operation.*mismatched types" + + _ = e == g // ERROR "invalid operation.*not defined" + _ = e != g // ERROR "invalid operation.*not defined" + _ = e >= g // ERROR "invalid operation.*not defined" + _ = g == e // ERROR "invalid operation.*not defined" + _ = g != e // ERROR "invalid operation.*not defined" + _ = g >= e // ERROR "invalid operation.*not defined" + + _ = i == g // ERROR "invalid operation.*not defined" + _ = i != g // ERROR "invalid operation.*not defined" + _ = i >= g // ERROR "invalid operation.*not defined" + _ = g == i // ERROR "invalid operation.*not defined" + _ = g != i // ERROR "invalid operation.*not defined" + _ = g >= i // ERROR "invalid operation.*not defined" + + _ = _ == e // ERROR "cannot use _ as value" + _ = _ == i // ERROR "cannot use _ as value" + _ = _ == c // ERROR "cannot use _ as value" + _ = _ == n // ERROR "cannot use _ as value" + _ = _ == f // ERROR "cannot use _ as value" + _ = _ == g // ERROR "cannot use _ as value" + + _ = e == _ // ERROR "cannot use _ as value" + _ = i == _ // ERROR "cannot use _ as value" + _ = c == _ // ERROR "cannot use _ as value" + _ = n == _ // ERROR "cannot use _ as value" + _ = f == _ // ERROR "cannot use _ as value" + _ = g == _ // ERROR "cannot use _ as value" + + _ = _ == _ // ERROR "cannot use _ as value" + + _ = e ^ c // ERROR "invalid operation.*mismatched types" + _ = c ^ e // ERROR "invalid operation.*mismatched types" + _ = 1 ^ e // ERROR "invalid operation.*mismatched types" + _ = e ^ 1 // ERROR "invalid operation.*mismatched types" + _ = 1 ^ c + _ = c ^ 1 +) diff --git a/test/live.go b/test/live.go index f96bbcc6c0d..2f421066a5f 100644 --- a/test/live.go +++ b/test/live.go @@ -137,10 +137,7 @@ var i9 interface{} func f9() bool { g8() x := i9 - // using complex number in comparison so that - // there is always a convT2E, no matter what the - // interface rules are. - return x != 99.0i // ERROR "live at call to convT2E: x" + return x != interface{}(99.0i) // ERROR "live at call to convT2E: x" } // liveness formerly confused by UNDEF followed by RET,