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

Jump to bottom

cmd/compile: reuse boxed primitive in slice literals of interfaces #29574

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

cmd/compile: reuse boxed primitive in slice literals of interfaces #29574

dsnet opened this issue Jan 4, 2019 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Jan 4, 2019 •
edited by josharian

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
@randall77
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@josharian josharian mentioned this issue Jan 11, 2019
Open
@julieqiu julieqiu added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 28, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@josharian @rsc @andybons @julieqiu @dsnet @randall77