Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: optimize single return value type assertion #17405

Closed
valyala opened this issue Oct 11, 2016 · 5 comments
Closed

cmd/compile: optimize single return value type assertion #17405

valyala opened this issue Oct 11, 2016 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@valyala
Copy link
Contributor

valyala commented Oct 11, 2016

This is a follow-up for #17118 .

Currently in go tip single return value type assertion is much slower comparing to type assertion returning two values:

func BenchmarkTypeAssertSingleReturnValue(b *testing.B) {
        n := 42
        v := interface{}(n)
        for i := 0; i < b.N; i++ {
                a := v.(int)
                if a != n {
                        b.Fatalf("unexpected a: %d. Expecting %d", a, n)
                }
        }
}

func BenchmarkTypeAssertTwoReturnValues(b *testing.B) {
        n := 42
        v := interface{}(n)
        for i := 0; i < b.N; i++ {
                a, ok := v.(int)
                if !ok {
                        b.Fatalf("type assertion failed for v=%v", v)
                }
                if a != n {
                        b.Fatalf("unexpected a: %d. Expecting %d", a, n)
                }
        }
}

Results:

BenchmarkTypeAssertSingleReturnValue-4      200000000            7.97 ns/op        0 B/op          0 allocs/op
BenchmarkTypeAssertTwoReturnValues-4        2000000000           1.09 ns/op        0 B/op          0 allocs/op

The compiler may optimize single return value type assertion (x := v.(T)) by rewriting it into the following code:

x, ok := v.(T)
if !ok {
    panic(...)
}
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 11, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Oct 11, 2016
@valyala
Copy link
Contributor Author

valyala commented Oct 15, 2016

cc'ing @randall77

@kvu787
Copy link
Contributor

kvu787 commented Oct 15, 2016

I've found that checked scalar type assertions:

x, ok := y.(T) // T is scalar type

are much faster than other type assertions.

This commit inlines checked scalar type assertions: 615a52b

Before commit benchmarks:

BenchmarkTypeAssertSingleReturnValue-4    200000000         10.0 ns/op
BenchmarkTypeAssertTwoReturnValues-4      100000000         10.4 ns/op

After commit benchmarks:

BenchmarkTypeAssertSingleReturnValue-4    100000000         11.1 ns/op
BenchmarkTypeAssertTwoReturnValues-4      2000000000         1.54 ns/op

Can unchecked scalar type assertions (x := y.(int)) also be inlined?

@randall77
Copy link
Contributor

I'll see if I can get in a fix for this before the freeze.

@randall77 randall77 self-assigned this Oct 15, 2016
@gopherbot
Copy link

CL https://golang.org/cl/32416 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 31, 2016
We used to have to keep on-stack copies of these types.
Now they can be registerized.

[0]T is kind of trivial but might as well handle it.

This change enables another change I'm working on to improve how x.(T)
expressions are handled (#17405).  This CL helps because now all
types that are direct interface types are registerizeable (e.g. [1]*byte).

No higher-degree arrays for now because non-constant indexes are hard.

Update #17405

Change-Id: I2399940965d17b3969ae66f6fe447a8cefdd6edd
Reviewed-on: https://go-review.googlesource.com/32416
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

CL https://golang.org/cl/32313 mentions this issue.

@golang golang locked and limited conversation to collaborators Nov 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants