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

net: sloppy struct field arrangement could be re-arranged for more compact structs #42415

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

Comments

@odeke-em
Copy link
Member

odeke-em commented Nov 6, 2020

Coming here from tip ecc3f51, with one of the static analysis tools, we have developed at Orijtech, Inc., we've found 2
fields in the net package that could be re-arranged for optimal sizes, hence the usage of "sloppy struct field arrangement".

And here they are

net/dnsconfig_unix.go:23:16: struct has size 152 (size class 160), could be 144 (size class 144), rearrange to 
struct{mtime time.Time; search []string; servers []string; lookup []string; err error; timeout time.Duration;
attempts int; ndots int; soffset uint32; unknownOpt bool; rotate bool; singleRequest bool; useTCP bool}
for optimal size (10.00% savings)
net/dnsclient_unix_test.go:50:35: struct has size 296 (size class 320), could be 288 (size class 288),
rearrange to struct{server string; timeout int; question dnsmessage.Question; rcode dnsmessage.RCode}
for optimal size (10.00% savings)
@davecheney
Copy link
Contributor

I don’t think there is a lot of value of analysing _test.go files.

@cuonglm
Copy link
Member

cuonglm commented Nov 6, 2020

I don’t think there is a lot of value of analysing _test.go files.

Sure, It's just an analysis pass, so I think it currently does the same as what analysis passes do. Is there an option for analysis pass skip test files 🤔

@odeke-em
Copy link
Member Author

odeke-em commented Nov 6, 2020

I don’t think there is a lot of value of analysing _test.go files.

@davecheney does have a point, I thought about that too, but we were trying to be as thorough as possible!
@cuonglm let's skip _test.go files, and perhaps make them an explicit opt-in instead.

@toothrot toothrot 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
@toothrot toothrot added this to the Backlog milestone Nov 6, 2020
@toothrot
Copy link
Contributor

toothrot commented Nov 6, 2020

/cc @bradfitz @ianlancetaylor

@rsc
Copy link
Contributor

rsc commented Nov 6, 2020

The struct identified here - type dnsConfig - is something that there's typically only one of in an entire program. The savings of 16 bytes in an entire program, at the cost of making the struct field list less natural to read, is not worth it.

Echoing what I wrote on #42412, you can improve the targeting of your suggestions by combining your tool with memory profiles. It's important not to make changes to code that doesn't actually have any benefit to being changed. That can only introduce bugs - all risk, no reward.

@rsc
Copy link
Contributor

rsc commented Nov 7, 2020

Given that dnsConfig shouldn't be changed and the other is in a test and shouldn't be changed, closing this.

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

No branches or pull requests

6 participants