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: reuse boxed primitive in slice literals of interfaces #29574

Open
dsnet opened this Issue Jan 4, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@dsnet
Copy link
Member

dsnet commented Jan 4, 2019

Using go1.12

Consider the following snippet:

func main() {
	lit := []interface{}{
		0, 0, 0, 0, 0,
	}
	type ifaceHeader struct{ Type, Data unsafe.Pointer }
	for i := range lit {
		fmt.Printf("%+v\n", *(*ifaceHeader)(unsafe.Pointer(&lit[i])))
	}
}

This currently prints:

{Type:0x4962e0 Data:0x4c95c0}
{Type:0x4962e0 Data:0x4c95c8}
{Type:0x4962e0 Data:0x4c95d0}
{Type:0x4962e0 Data:0x4c95d8}
{Type:0x4962e0 Data:0x4c95e0}

Which is indicative that each boxed integer is getting it's own heap allocated element (since the data pointer is different for every element). It seems that the compiler should be able to reuse boxed elements that are of the same value.

Furthermore, I would expect this work across different types with the same underlying kind. Thus, I would also expect:

type EnumC int32
type EnumB int32
type EnumC int32
lit := []interface{EnumA(0), EnumB(0), EnumC(0)} 

to all share the same boxed element since all three types are of the int32 kind.

\cc @randall77

@dsnet dsnet added the Performance label Jan 4, 2019

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Jan 4, 2019

The backing store for each interface data item is currently its own static temp.
Since we don't write to those static temps, it would be ok to share them.
In order to do so we'd have to name them with their contents so the linker can merge them. That shouldn't be hard; I think we already do that for the backing of strings.

As a side note, we could use the zero value (runtime.zeroVal) for this particular case.

@randall77 randall77 added this to the Go1.13 milestone Jan 4, 2019

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Jan 4, 2019

Which is indicative that each boxed integer is getting it's own heap allocated element

BTW, these are not heap-allocated elements, but statically allocated elements (in the data section).

@josharian

This comment has been minimized.

Copy link
Contributor

josharian commented Jan 5, 2019

Another observation. Here's a snippet of asm:

	0x004e 00078 (x.go:10)	LEAQ	type.int(SB), AX
	0x0055 00085 (x.go:10)	MOVQ	AX, ""..autotmp_12+104(SP)
	0x005a 00090 (x.go:10)	PCDATA	$2, $3
	0x005a 00090 (x.go:10)	LEAQ	"".statictmp_0(SB), CX
	0x0061 00097 (x.go:10)	PCDATA	$2, $2
	0x0061 00097 (x.go:10)	MOVQ	CX, ""..autotmp_12+112(SP)
	0x0066 00102 (x.go:10)	MOVQ	AX, ""..autotmp_12+120(SP)
	0x006b 00107 (x.go:10)	PCDATA	$2, $3
	0x006b 00107 (x.go:10)	LEAQ	"".statictmp_1(SB), CX
	0x0072 00114 (x.go:10)	PCDATA	$2, $2
	0x0072 00114 (x.go:10)	MOVQ	CX, ""..autotmp_12+128(SP)
	0x007a 00122 (x.go:10)	MOVQ	AX, ""..autotmp_12+136(SP)
	0x0082 00130 (x.go:10)	PCDATA	$2, $3
	0x0082 00130 (x.go:10)	LEAQ	"".statictmp_2(SB), CX

We are painstakingly filling out lit, one word at a time.

For some sizes and types of lit, we could instead lay out the entire data structure (duplicating the type word many times) and then do a DUFFCOPY or the like to copy it to the stack. It'd be faster to compile, probably faster to execute, perhaps smaller binary size.

@josharian

This comment has been minimized.

Copy link
Contributor

josharian commented Jan 5, 2019

Ah, @randall77 made that same suggestion over at
#29573 (comment).

Also related: #21561, #19818.

Since we don't write to those static temps, it would be ok to share them.

FWIW, I tried this in https://go-review.googlesource.com/c/go/+/42170/ and got stuck. So that's a starting place if anyone wants to tackle this.

@josharian

This comment has been minimized.

Copy link
Contributor

josharian commented Jan 5, 2019

As a side note, we could use the zero value (runtime.zeroVal) for this particular case.

And this is #23948. We keep re-discovering this whole suit of issues and possible solutions. We just haven't fixed them. :P

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