FAQ
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/

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)

Search Discussions

  • Brad Fitzpatrick at Nov 4, 2012 at 11:05 pm
    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, 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)

  • Daniel Morsing at Nov 5, 2012 at 6:11 am

    On Mon, Nov 5, 2012 at 12:05 AM, Brad Fitzpatrick 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 at Nov 6, 2012 at 6:41 pm
    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#newcode284
    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#newcode352
    src/pkg/runtime/iface.c:352: if(t != e.type) {
    ok = t==e.type;
    FLUSH(&ok);

    https://codereview.appspot.com/6812079/
  • Daniel Morsing at Nov 6, 2012 at 7:37 pm
    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#newcode284
    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#newcode352
    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.

    https://codereview.appspot.com/6812079/
  • Daniel Morsing at Nov 6, 2012 at 7:40 pm
    *** 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


    http://codereview.appspot.com/6812079/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 4, '12 at 2:56p
activeNov 6, '12 at 7:40p
posts6
users3
websitegolang.org

People

Translate

site design / logo © 2021 Grokbase