fieldalignment: struct with 24 pointer bytes could be 16 (govet)

[frederikhors]

Let's say I have this struct:

type MyStruct struct {
	id       uint64
	required string
	note     *string
}

If I use gopium (from VSCode, clicking gopium pack on the struct) I get this:

type MyStruct struct {
	required string
	id       uint64
	note     *string
}

which isn't good for govet's fieldalignment linter in golangci-lint which is erroring with:

fieldalignment: struct with 24 pointer bytes could be 16 (govet)

Is this correct?

Where is my fault?

The error with govet goes away if I use this struct instead:

type MyStruct struct {
	note     *string
	required string
	id       uint64
}

Can you help me understand?

[frederikhors]

golangci/golangci-lint#2298

[1pkg]

Hello @frederikhors, thank you for reporting this. I read fieldalignment code https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.7:go/analysis/passes/fieldalignment/fieldalignment.go to understand why your example is getting failing the linter.

If you look closely, on the line https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.7:go/analysis/passes/fieldalignment/fieldalignment.go;l=85, you can notice that NOT the struct of size %d could be %d condition is getting triggered, but rather the next condition struct with %d pointer bytes could be %d. So the size of the structure is packed in both scenarios which can be also proven by this example https://play.golang.org/p/6Zg50c34qeu.

Although the next check struct with %d pointer bytes could be %d is interesting one and indeed is triggered. Which I believe is referring to how much aligned pointer data is present in structure indirectly. They using next procedure https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.7:go/analysis/passes/fieldalignment/fieldalignment.go;l=324;drc=9b675d0446711ebd43b51946d417b60a4ca5a6e3 to evaluate pointer size. With combination of https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.7:go/analysis/passes/fieldalignment/fieldalignment.go;drc=9b675d0446711ebd43b51946d417b60a4ca5a6e3;l=145 to evaluate the optimal order of the structure. Currently gopium uses simplified version of the optimalOrder procedure and doesn't sort pointer fields in a special way, which might be a mistake.

I need to evaluate this struct with pointers bytes check deeper, and potentially update gopium optimalOrder to one from fieldalignment checker.

[frederikhors]

I need to evaluate this struct with pointers bytes check deeper, and potentially update gopium optimalOrder to one from fieldalignment checker.

Yes, please and thanks.

I don't quite understand what it refers to.

[1pkg]

I think I delved to the bottom of the fieldalignment check finally. The second part of check struct with %d pointer bytes could be %d isn't related to the size of the final struct anyhow. In fact, from what I understand, it's related only to how GC works in Go. In mark phase GC uses the size of ptrdata as an optimization technique to scan only necessary bytes in objects and avoid full scan, see https://github.com/golang/go/blob/master/src/runtime/mgcmark.go#L823. So instead of scanning all object bytes for optimization it scans only up to the last pointer boundary in the structure [start, ptrdata). So this is why putting the pointer fields prior to non pointer fields matters in Go.

// GC will scan all 808 bytes.
type gcNotOptimized struct {
  a [100]int64
  b *int64 
}

// GC will scan only 8 bytes.
type gcNotOptimized struct {
  b *int64 
  a [100]int64
}

[1pkg]

This is indeed quite important performance aspect. So I will port the optimalOrder code from https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.7:go/analysis/passes/fieldalignment/fieldalignment.go;drc=9b675d0446711ebd43b51946d417b60a4ca5a6e3;l=145 for the pack strategy in gopium in upcoming days.