code review 6812079: cmd/gc, runtime: avoid unnecessary copy on type assertion.

cmd/gc, runtime: avoid unnecessary copy on type assertion.

When the first result of a type assertion is blank, the compiler would still copy out a potentially large non-interface type.

Fixes issue 1021.

[DMorsing, 2012-11-04 14:56:33]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2012-11-04 23:05:21]

Out of curiosity, have you tried to simplify the regexp package as the
original issue discussed?  If so, I'd be curious if the performance was
unchanged.

On Sun, Nov 4, 2012 at 3:56 PM, <daniel.morsing@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/gc, runtime: avoid unnecessary copy on type assertion.
>
> When the first result of a type assertion is blank, the compiler would
> still copy out a potentially large non-interface type.
>
> Fixes issue 1021.
>
> Please review this at
http://codereview.appspot.com/**6812079/<http://codereview.appspot.com/6812079/>
>
> Affected files:
>   M src/cmd/gc/builtin.c
>   M src/cmd/gc/runtime.go
>   M src/cmd/gc/walk.c
>   M src/pkg/runtime/iface.c
>
>
> Index: src/cmd/gc/builtin.c
> ==============================**==============================**=======
> --- a/src/cmd/gc/builtin.c
> +++ b/src/cmd/gc/builtin.c
> @@ -54,6 +54,8 @@
>         "func @\"\".assertI2I2(@\"\".typ *byte, @\"\".iface any)
> (@\"\".ret any, @\"\".ok bool)\n"
>         "func @\"\".assertI2T(@\"\".typ *byte, @\"\".iface any) (@\"\".ret
> any)\n"
>         "func @\"\".assertI2T2(@\"\".typ *byte, @\"\".iface any)
> (@\"\".ret any, @\"\".ok bool)\n"
> +       "func @\"\".assertI2TOK(@\"\".typ *byte, @\"\".iface any)
> (@\"\".ok bool)\n"
> +       "func @\"\".assertE2TOK(@\"\".typ *byte, @\"\".iface any)
> (@\"\".ok bool)\n"
>         "func @\"\".ifaceeq(@\"\".i1 any, @\"\".i2 any) (@\"\".ret bool)\n"
>         "func @\"\".efaceeq(@\"\".i1 any, @\"\".i2 any) (@\"\".ret bool)\n"
>         "func @\"\".ifacethash(@\"\".i1 any) (@\"\".ret uint32)\n"
> Index: src/cmd/gc/runtime.go
> ==============================**==============================**=======
> --- a/src/cmd/gc/runtime.go
> +++ b/src/cmd/gc/runtime.go
> @@ -76,6 +76,8 @@
>  func assertI2I2(typ *byte, iface any) (ret any, ok bool)
>  func assertI2T(typ *byte, iface any) (ret any)
>  func assertI2T2(typ *byte, iface any) (ret any, ok bool)
> +func assertI2TOK(typ *byte, iface any) (ok bool)
> +func assertE2TOK(typ *byte, iface any) (ok bool)
>
>  func ifaceeq(i1 any, i2 any) (ret bool)
>  func efaceeq(i1 any, i2 any) (ret bool)
> Index: src/cmd/gc/walk.c
> ==============================**==============================**=======
> --- a/src/cmd/gc/walk.c
> +++ b/src/cmd/gc/walk.c
> @@ -686,6 +686,31 @@
>                 n->ninit = nil;
>                 r = n->rlist->n;
>                 walkexprlistsafe(n->list, init);
> +               if(isblank(n->list->n) && !isinter(r->type)) {
> +                       strcpy(buf, "assert");
> +                       p = buf+strlen(buf);
> +                       if(isnilinter(r->left->type))
> +                               *p++ = 'E';
> +                       else
> +                               *p++ = 'I';
> +                       *p++ = '2';
> +                       *p++ = 'T';
> +                       *p++ = 'O';
> +                       *p++ = 'K';
> +                       *p = '\0';
> +
> +                       fn = syslook(buf, 1);
> +                       ll = list1(typename(r->type));
> +                       ll = list(ll, r->left);
> +                       argtype(fn, r->left->type);
> +                       n1 = nod(OCALL, fn, N);
> +                       n1->list = ll;
> +                       n = nod(OAS, n->list->next->n, n1);
> +                       typecheck(&n, Etop);
> +                       walkexpr(&n, init);
> +                       goto ret;
> +               }
> +
>                 r->op = ODOTTYPE2;
>                 walkexpr(&r, init);
>                 ll = ascompatet(n->op, n->list, &r->type, 0, init);
> Index: src/pkg/runtime/iface.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/iface.c
> +++ b/src/pkg/runtime/iface.c
> @@ -278,6 +278,18 @@
>         copyout(t, &i.data, ret);
>  }
>
> +void
> +runtime·assertI2TOK(Type *t, Iface i, bool ok)
> +{
> +       if(i.tab == nil || i.tab->type != t) {
> +               ok = false;
> +               FLUSH(&ok);
> +               return;
> +       }
> +       ok = true;
> +       FLUSH(&ok);
> +}
> +
>  static void assertE2Tret(Type *t, Eface e, byte *ret);
>
>  // func ifaceE2T(typ *byte, iface any) (ret any)
> @@ -334,6 +346,18 @@
>         copyout(t, &e.data, ret);
>  }
>
> +void
> +runtime·assertE2TOK(Type *t, Eface e, bool ok)
> +{
> +       if(t != e.type) {
> +               ok = false;
> +               FLUSH(&ok);
> +               return;
> +       }
> +       ok = true;
> +       FLUSH(&ok);
> +}
> +
>  // func convI2E(elem any) (ret any)
>  void
>  runtime·convI2E(Iface i, Eface ret)
>
>
>

[DMorsing, 2012-11-05 06:11:56]

On Mon, Nov 5, 2012 at 12:05 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Out of curiosity, have you tried to simplify the regexp package as the
> original issue discussed?  If so, I'd be curious if the performance was
> unchanged.
>

The mentioned construct doesn't exist any more. This is a very old issue.

[rsc, 2012-11-06 18:41:40]

LGTM

https://codereview.appspot.com/6812079/diff/5001/src/pkg/runtime/iface.c
File src/pkg/runtime/iface.c (right):

https://codereview.appspot.com/6812079/diff/5001/src/pkg/runtime/iface.c#newc...
src/pkg/runtime/iface.c:284: if(i.tab == nil || i.tab->type != t) {
ok = i.tab!=nil && i.tab->type==t;
FLUSH(&ok);

same kind of thing below.

It might even make sense to inline these but this is a good step for now.

https://codereview.appspot.com/6812079/diff/5001/src/pkg/runtime/iface.c#newc...
src/pkg/runtime/iface.c:352: if(t != e.type) {
ok = t==e.type;
FLUSH(&ok);

[DMorsing, 2012-11-06 19:37:11]

https://codereview.appspot.com/6812079/diff/5001/src/pkg/runtime/iface.c
File src/pkg/runtime/iface.c (right):

https://codereview.appspot.com/6812079/diff/5001/src/pkg/runtime/iface.c#newc...
src/pkg/runtime/iface.c:284: if(i.tab == nil || i.tab->type != t) {
On 2012/11/06 18:41:40, rsc wrote:
> ok = i.tab!=nil && i.tab->type==t;
> FLUSH(&ok);
> 
> same kind of thing below.
> 
> It might even make sense to inline these but this is a good step for now.

Done.

https://codereview.appspot.com/6812079/diff/5001/src/pkg/runtime/iface.c#newc...
src/pkg/runtime/iface.c:352: if(t != e.type) {
On 2012/11/06 18:41:40, rsc wrote:
> ok = t==e.type;
> FLUSH(&ok);

Done.

[DMorsing, 2012-11-06 19:40:45]

*** Submitted as http://code.google.com/p/go/source/detail?r=ae0310fd287b ***

cmd/gc, runtime: avoid unnecessary copy on type assertion.

When the first result of a type assertion is blank, the compiler would still
copy out a potentially large non-interface type.

Fixes issue 1021.

R=golang-dev, bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/6812079