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: elide useless type assertion #25189

Open
dsnet opened this Issue Apr 30, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@dsnet
Member

dsnet commented Apr 30, 2018

Consider the following:

type CustomUnmarshaler interface {
	CustomUnmarshal([]byte) error
}

type MyMessage struct{}

func (m *MyMessage) Unmarshal(b []byte) error {
	if u, ok := (interface{})(m).(CustomUnmarshaler); ok {
		return u.CustomUnmarshal(b)
	}
	return nil
}

In this situation, the compiler knows that the MyMessage type has no CustomUnmarshal method, so the type assertion cannot possibly succeed. In this case, the condition of the if statement can be statically determined and the body be considered dead code.

However, a tip build of the compiler continues to emit code for the assertion.

0x0021 00033 (/tmp/main.go:10)	LEAQ	type."".CustomUnmarshaler(SB), AX
0x0028 00040 (/tmp/main.go:10)	MOVQ	AX, (SP)
0x002c 00044 (/tmp/main.go:10)	LEAQ	type.*"".Foo(SB), AX
0x0033 00051 (/tmp/main.go:10)	MOVQ	AX, 8(SP)
0x0038 00056 (/tmp/main.go:9)	MOVQ	"".f+64(SP), AX
0x003d 00061 (/tmp/main.go:10)	MOVQ	AX, 16(SP)
0x0042 00066 (/tmp/main.go:10)	PCDATA	$0, $1
0x0042 00066 (/tmp/main.go:10)	CALL	runtime.assertE2I2(SB)
0x0047 00071 (/tmp/main.go:10)	MOVQ	24(SP), AX
0x004c 00076 (/tmp/main.go:10)	MOVQ	32(SP), CX
0x0051 00081 (/tmp/main.go:10)	LEAQ	40(SP), DX
0x0056 00086 (/tmp/main.go:10)	CMPB	(DX), $0
0x0059 00089 (/tmp/main.go:10)	JEQ	161
0x005b 00091 (/tmp/main.go:11)	MOVQ	24(AX), AX
0x005f 00095 (/tmp/main.go:11)	MOVQ	"".b+72(SP), DX
0x0064 00100 (/tmp/main.go:11)	MOVQ	DX, 8(SP)
0x0069 00105 (/tmp/main.go:11)	MOVQ	"".b+80(SP), DX
0x006e 00110 (/tmp/main.go:11)	MOVQ	DX, 16(SP)
0x0073 00115 (/tmp/main.go:11)	MOVQ	"".b+88(SP), DX
0x0078 00120 (/tmp/main.go:11)	MOVQ	DX, 24(SP)
0x007d 00125 (/tmp/main.go:11)	MOVQ	CX, (SP)
0x0081 00129 (/tmp/main.go:11)	PCDATA	$0, $2
0x0081 00129 (/tmp/main.go:11)	CALL	AX
0x0083 00131 (/tmp/main.go:11)	MOVQ	32(SP), AX
0x0088 00136 (/tmp/main.go:11)	MOVQ	40(SP), CX
0x008d 00141 (/tmp/main.go:11)	MOVQ	AX, "".~r1+96(SP)
0x0092 00146 (/tmp/main.go:11)	MOVQ	CX, "".~r1+104(SP)
0x0097 00151 (/tmp/main.go:11)	MOVQ	48(SP), BP
0x009c 00156 (/tmp/main.go:11)	ADDQ	$56, SP
0x00a0 00160 (/tmp/main.go:11)	RET

@dsnet dsnet added the Performance label Apr 30, 2018

@rasky

This comment has been minimized.

Member

rasky commented Apr 30, 2018

I assume these things happen while inlining (and as we get more aggressive in inlining, they will be more and more common).

The type conversion is lowered during buildssa into runtime calls. I've already attempted to write some code to delay lowering in a similar case, but I couldn't find a good way to layout a runtime function call after buildssa (eg: stack slots). It's way above my head.

It would help if somebody more familiar with the compiler could come up with some utility to add a function call during a SSA pass. That would be very useful for me to start working towards delaying lowering of runtime calls, exploiting optimizations opportunities that higher-level semantics bring.

@bcmills bcmills added this to the Unplanned milestone Apr 30, 2018

@bcmills

This comment has been minimized.

Member

bcmills commented Apr 30, 2018

How much real-world code actually does these impossible assertions? (That is, what's the potential impact of the optimization?)

I assume that this mostly occurs in generated code, but in that case wouldn't it be even better — particularly in terms of compile times — to avoid emitting the redundant assertions in the first place?

@CAFxX

This comment has been minimized.

Contributor

CAFxX commented May 1, 2018

@bcmills the point that @rasky makes about this happening because of inlining I think is probably the main source of these occurrences already today. See e.g. https://godbolt.org/g/GzDvaU or https://godbolt.org/g/N5W6Ua (in both cases the call to f() is inlined, but the type assertion remains)

(Also, but this is a vastly less important argument in the context of this discussion, requiring code generators to be that smart would vastly complicate them)

@navytux

This comment has been minimized.

Contributor

navytux commented May 1, 2018

Tracking concrete type behind an interface will also help #19361, thus lowering call overhead if methods are invoked, and potentially lowering GC overhead through not letting call arguments to escape.

@rasky

This comment has been minimized.

Member

rasky commented May 1, 2018

I think that, wrt inlining, we're in a sort of catch-22: inlining does not seem to help much because we're bad at exploiting many of these opportunities in our optimizers; and adding optimizations like this one is going to have a tool speed impact without great benefits because we don't inline nearly as much as we should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment