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: empty struct takes space #12884

Closed
minux opened this issue Oct 8, 2015 · 14 comments
Closed

cmd/compile: empty struct takes space #12884

minux opened this issue Oct 8, 2015 · 14 comments

Comments

@minux
Copy link
Member

minux commented Oct 8, 2015

Found while experimenting with https://golang.org/cl/15522.

Empty field (_ struct{}) takes space in Go 1.5, but not in Go 1.4
or any previous Go releases. See http://play.golang.org/p/OTr-GLz2UN.

$ go run OTr-GLz2UN.go # Go tip
16 24
16 16
12
$ go143 run OTr-GLz2UN.go
16 16
16 16
8

It's a compiler regression introduced by Go 1.5.

Tentatively labeled Go 1.5.2 as this can't be workaround.

@minux minux self-assigned this Oct 8, 2015
@minux minux added this to the Go1.5.2 milestone Oct 8, 2015
@bradfitz
Copy link
Contributor

bradfitz commented Oct 8, 2015

Nice find!

@mdempsky
Copy link
Member

mdempsky commented Oct 9, 2015

In general, they do not take space. E.g., this program prints 4:

package main

import (
    "fmt"
    "unsafe"
)

func main() {
    fmt.Println(unsafe.Sizeof(struct {
        _ struct{}
        x int32
    }{}))
}

The change to pad out structs that have a final zero-size field was to fix issue #9401. I think this is working as intended.

@bradfitz
Copy link
Contributor

bradfitz commented Oct 9, 2015

So we should change

type ProgInfo struct {
        Flags    uint32   // flag bits                                                                             
        Reguse   uint64   // registers implicitly used by this instruction                                         
        Regset   uint64   // registers implicitly set by this instruction                                          
        Regindex uint64   // registers used by addressing mode                                                     
        _        struct{} // to prevent unkeyed literals                                                           
}

to

type ProgInfo struct {
        _        struct{} // to prevent unkeyed literals                                                           
        Flags    uint32   // flag bits                                                                             
        Reguse   uint64   // registers implicitly used by this instruction                                         
        Regset   uint64   // registers implicitly set by this instruction                                          
        Regindex uint64   // registers used by addressing mode                                                     
}

... I guess?

@mdempsky
Copy link
Member

mdempsky commented Oct 9, 2015

That should work to avoid the padding.

@minux minux removed this from the Go1.5.2 milestone Oct 9, 2015
@minux
Copy link
Member Author

minux commented Oct 9, 2015 via email

@davecheney
Copy link
Contributor

Moving the _ field to the top of the struct SGTM.

On Fri, 9 Oct 2015 11:10 Minux Ma notifications@github.com wrote:

ahh, I forgot about that trailing zero-sized field issue entirely.
thanks for pointing it out.

precise GC and zero-sized fields are subtle.


Reply to this email directly or view it on GitHub
#12884 (comment).

@davecheney
Copy link
Contributor

Perhaps instead of moving the _ field, augmenting the comment to mention
that zero sized fields consume 1 byte of space, however that allocation is
subsumed into the padding for the int32.

However on 32 bit platforms, leaving the zero sized field at the end will
cause it to consume 4 bytes, so maybe it does need to move to the top of
the struct.

On Fri, 9 Oct 2015 11:12 Dave Cheney dave@cheney.net wrote:

Moving the _ field to the top of the struct SGTM.

On Fri, 9 Oct 2015 11:10 Minux Ma notifications@github.com wrote:

ahh, I forgot about that trailing zero-sized field issue entirely.
thanks for pointing it out.

precise GC and zero-sized fields are subtle.


Reply to this email directly or view it on GitHub
#12884 (comment).

@mdempsky
Copy link
Member

mdempsky commented Oct 9, 2015

It won't take any space at the start of the struct.

@gopherbot
Copy link

CL https://golang.org/cl/15660 mentions this issue.

@mdempsky
Copy link
Member

mdempsky commented Oct 9, 2015

In case anyone's interested in investigating more structs that might benefit from reordering their fields, I put together github.com/mdempsky/maligned as a quick program to calculate struct sizes using cmd/compile's field layout rules, and compare against what they would be if the fields were sorted.

Example:

$ maligned cmd/internal/obj
/usr/local/google/home/mdempsky/wd/go/src/cmd/internal/obj/link.go:249:15: struct of size 40 could be 32
/usr/local/google/home/mdempsky/wd/go/src/cmd/internal/obj/link.go:476:11: struct of size 584 could be 568

@davecheney
Copy link
Contributor

@minux @mdempsky

ahh, I forgot about that trailing zero-sized field issue entirely.
thanks for pointing it out.

How can one take the address of an unnamed field in a struct, zero sized or not ? In #9401 the type in question was

type T struct {
    A int
    Y struct{}
}

But in the example above the field is unnamed and inaccessible.

@mdempsky
Copy link
Member

mdempsky commented Oct 9, 2015

It's possible at least via package reflect: http://play.golang.org/p/esuSUoKomT

@davecheney
Copy link
Contributor

@mdempsky, yes of course, thank you for explaining to me.

@josharian
Copy link
Contributor

@minux minux closed this as completed in 30b9663 Oct 13, 2015
valyala referenced this issue in valyala/fasthttp Mar 5, 2016
This should help `go vet` detecting invalid structs' copyings.
See golang/go#8005 (comment) for details.
valyala added a commit to valyala/fasthttp that referenced this issue Mar 5, 2016
@golang golang locked and limited conversation to collaborators Oct 12, 2016
@rsc rsc unassigned minux Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants