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: liveness analysis conservative for compound objects #28626

Open
randall77 opened this Issue Nov 6, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@randall77
Contributor

randall77 commented Nov 6, 2018

func f(data []byte) {
	dataSize := len(data)
	g(data)
	runtime.GC()
	global = dataSize
}

func g([]byte)

var global int

The compiler reports (with -live) that data is live at the runtime.GC call. In fact, only the .len field of data is live at that point. But because we track liveness by variable, if any field of that variable is live whole variable is live. This means data will be retained unnecessarily by the garbage collector.

The generated code contains:

	0x001d 00029 (tmp1.go:7)	MOVQ	"".data+40(SP), AX
	0x0022 00034 (tmp1.go:7)	MOVQ	AX, (SP)
	0x0026 00038 (tmp1.go:7)	MOVQ	"".data+48(SP), AX
	0x002b 00043 (tmp1.go:7)	MOVQ	AX, 8(SP)
	0x0030 00048 (tmp1.go:7)	MOVQ	"".data+56(SP), CX
	0x0035 00053 (tmp1.go:7)	MOVQ	CX, 16(SP)
	0x003a 00058 (tmp1.go:7)	CALL	"".g(SB)
	0x003f 00063 (tmp1.go:8)	CALL	runtime.GC(SB)
	0x0044 00068 (tmp1.go:9)	MOVQ	"".data+48(SP), AX
	0x0049 00073 (tmp1.go:9)	MOVQ	AX, "".global(SB)

Even though the code looks like it reads the length first, the compiler postpones that read until after the runtime.GC call (so it doesn't have to issue a pointless spill).

Two solutions come to mind. One is to track liveness of individual pointer fields of variables, instead of whole variables. That would work, but could be expensive for variables which have lots of pointer fields. It also isn't possible to do precisely for variables which are arrays of pointers and have non-constant indexes.

The other solution is to ignore reads of scalars when doing the liveness analysis, as the results of the liveness analysis are only used by the GC. This is more of a hack but would be a simpler fix for the code in this issue.

@randall77 randall77 added this to the Go1.12 milestone Nov 6, 2018

@mdempsky

This comment has been minimized.

Member

mdempsky commented Nov 6, 2018

Ignoring reads of scalar fields during liveness analysis was my first thought too.

@randall77

This comment has been minimized.

Contributor

randall77 commented Nov 6, 2018

It's not trivial to check the scalarness of a read in plive.go.
Usually it's easy, just check the type of the result.
Some reads return value/memory tuples, like MOVQatomicload.
Trickier still is ADDQload, which may be adding a scalar from memory to a pointer and generating a pointer result. Just because the result is a pointer doesn't mean the value being read from the variable is a pointer.

If we could extract the offset+size of the read, we could check in the type of the variable directly.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Nov 6, 2018

If we could extract the offset+size of the read, we could check in the type of the variable directly.

Isn't the offset already in AuxInt? We'd just need to encode the size of the read in the ssa.Op's SymEffects (at least for reads up to word size).

@randall77

This comment has been minimized.

Contributor

randall77 commented Nov 6, 2018

Yeah, the offset info is there. We'd have to switch on the aux type, there are a few others like symValAndOff.

@randall77

This comment has been minimized.

Contributor

randall77 commented Nov 27, 2018

Punting to 1.13.

@randall77 randall77 modified the milestones: Go1.12, Go1.13 Nov 27, 2018

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