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: struct field alive too long #24263

Open
randall77 opened this issue Mar 6, 2018 · 8 comments
Open

cmd/compile: struct field alive too long #24263

randall77 opened this issue Mar 6, 2018 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@randall77
Copy link
Contributor

Program 1:

package main

import (
	"fmt"
	"runtime"
)

func main() {
	y := make([]int, 1e6)
	runtime.GC()
	var stat runtime.MemStats
	runtime.ReadMemStats(&stat)
	fmt.Println(stat.Alloc)
	fmt.Println(len(y))
}

Program 2:

package main

import (
	"fmt"
	"runtime"
)

func main() {
	type T struct {
		x int
		y *[1e6]int
	}
	t := T{x: 7, y: new([1e6]int)}
	runtime.GC()
	var stat runtime.MemStats
	runtime.ReadMemStats(&stat)
	fmt.Println(stat.Alloc)
	fmt.Println(t.x)
}

In program 1, the allocation at make is garbage collected, so program 1 prints just ~100KB of usage.
In program 2, the allocation at new is not garbage collected, so it prints ~8MB.

The allocation in program 2 should be GCd, it is no longer referenced.
For some reason the compiler is keeping all of t alive even though only one field of it is alive after GC.
Strangely, if I replace fmt.Println with println, then the allocation gets GCd.
I'm not entirely sure what is going on yet, but it looks like t is being treated as addressable. But it is getting decomposed correctly. Strange.

First reported here: https://groups.google.com/forum/#!topic/golang-nuts/4qmn5gw7OBQ

@cherrymui
Copy link
Member

fmt.Println takes interface, so t.x is converted to interface, where its address is taken (passed to convT2E64). println doesn't take interface. Maybe this is the reason?

@gua-pian
Copy link

gua-pian commented Mar 6, 2018

In program2, when t passed to fmt.Println(), it is treated as interface{}, as an interface, it should keep its real value and its type descriptor.

@go101
Copy link

go101 commented Mar 6, 2018

@cherrymui the address of a copy of t.x will be taken. It should has no relations to t itself.
@dong-hao in program 2, it is t.x instead of t is passed to fmt.Println.

@gua-pian
Copy link

gua-pian commented Mar 6, 2018

@go101 you are right.
Consider that t.x is not a variable independent, when pass it to fmt.Println(), t should pass or keep in one piece.I have try

y := t.x
fmt.Println(y)

And the memory usage is low as program1.

@cherrymui
Copy link
Member

@go101
Even the interface value actually holds the address of a copy of t.x, when making the interface, it is the address of t.x that is passed to convT2E64. convT2E64 makes the copy.

package p

import "fmt"

func f() {
	type T struct {
		x int
		y *[1e6]int
	}
	t := T{x: 7, y: new([1e6]int)}
	fmt.Println(t.x)
}
	0x004c 00076 (x.go:11)	LEAQ	type.int(SB), AX
	0x0053 00083 (x.go:11)	MOVQ	AX, (SP)
	0x0057 00087 (x.go:11)	LEAQ	"".t+48(SP), AX
	0x005c 00092 (x.go:11)	MOVQ	AX, 8(SP)
	0x0061 00097 (x.go:11)	PCDATA	$0, $1
	0x0061 00097 (x.go:11)	CALL	runtime.convT2E64(SB)

Maybe we can change convT2E64 to take a uint64 value instead of pointer? I'm not sure about its impact and concerns, though.

@josharian
Copy link
Contributor

Maybe we can change convT2E64 to take a uint64 value instead of pointer?

That’s a good idea. Should be safe, because it is guaranteed to be scalar—pointer-shaped values don’t go through a convT2x call. Same probably holds for other specialized functions in runtime/iface.go, although for the multi-word ones (slice, string), it might be better to stick with an address to keep the call site small.

This would make a decent starter change for someone who wants to work on the runtime+compiler. Not trivial, but straightforward enough. Or you could just do it. :)

@randall77
Copy link
Contributor Author

That would solve this particular instance, but it doesn't solve the general problem of:

type T struct {
    a A
    b B
}
t := ...
fmt.Printf(t.b) // Retains pointers in t.a

But maybe that's ok and I'm just being too critical of a live variable analysis that is necessarily going to be conservative in some situations.

type T struct {
   a A
   b [4]byte
}
t := ...
x := t.b[:] // this reference will retain pointers in t.a

The thing that I think originally tripped me up is that fmt.Println(t.x) really doesn't look like we're taking the address of t. Maybe there is some way to detect that we're building an interface from part of a larger object and force a separate allocation in that case. I think this would only happen for stack allocations, so maybe forcing a copy of the underlying data before passing the address of it to convT2E and friends would work.

@josharian
Copy link
Contributor

Filed #24286 for Cherry's suggestion.

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 7, 2018
@andybons andybons added this to the Unplanned milestone Mar 7, 2018
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants