Closed frederikhors closed 2 years ago
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.
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.
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
}
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.
Let's say I have this struct:
If I use gopium (from VSCode, clicking
gopium pack
on the struct) I get this:which isn't good for
govet
'sfieldalignment
linter ingolangci-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:Can you help me understand?