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

Jump to bottom

fmt: sloppy struct field arrangement could be re-done for efficiency/RAM reduction #42413

Closed
odeke-em opened this issue Nov 6, 2020 · 3 comments
Closed

fmt: sloppy struct field arrangement could be re-done for efficiency/RAM reduction #42413

odeke-em opened this issue Nov 6, 2020 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@odeke-em
Copy link
Member

odeke-em commented Nov 6, 2020

Coming here from tip ecc3f51, with a tool we've developed at Orijtech, Inc, it reported

/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/fmt/format.go:41:10: struct has size 112,
could be 104, rearrange to struct{buf *buffer; wid int; prec int; intbuf [68]byte; fmtFlags} for
optimal size

because

go/src/fmt/format.go

Lines 41 to 52 in ecc3f51

type fmt struct {
buf *buffer
fmtFlags
wid int // width
prec int // precision
// intbuf is large enough to store %b of an int64 with a sign and
// avoids padding at the end of the struct on 32 bit architectures.
intbuf [68]byte
}

which when rearranged would become 7.14% more efficient as per https://play.golang.org/p/I8aIwwdbZYw.

Similar to #42412
@martisch
Copy link
Contributor

martisch commented Nov 6, 2020 •
edited

It is not sloppy but engineered this way, but not documented explicitly.

Ram:
Going from 112 to 104 bytes does not save Ram because the next lower size class is 96 bytes.

Efficiency:
I tried around with this alot in https://go-review.googlesource.com/c/go/+/20984 and back then it always made format slower potentially due to alignment change on the buffer or being more cache friendly at the end. I was using https://github.com/dominikh/go-structlayout back then.

We should benchmark fmt on amd64 and 386 before making any change to that struct.

@martisch martisch added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 6, 2020
@odeke-em
Copy link
Member Author

odeke-em commented Nov 6, 2020

Thanks @martisch, and you are right about the RAM-no-savings: we added size class recognition and it no longer complains. Should we close this issue then?

@martisch
Copy link
Contributor

martisch commented Nov 6, 2020

Since there are no ram savings I would say lets close this for now. Im not sure its worth toying with that structure ordering for performance gains. They would need to be verified outside microbenchmarks to make sure anyway.

@martisch martisch closed this as completed Nov 6, 2020
@golang golang locked and limited conversation to collaborators Nov 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@martisch @odeke-em @gopherbot