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

reorder fields to reduce size of objects #3008

Closed
wants to merge 4 commits into from

Conversation

kazzmir
Copy link

@kazzmir kazzmir commented Oct 17, 2024

This PR re-orders some fields in structs in order to reduce the size in bytes that the struct takes up in memory. Go is sensitive to the exact ordering of fields. I have not done any extensive profiling to see if this change makes a significant difference of memory usage at runtime, but this change is not harmful (unless you consider the ordering of fields useful for documentation purposes).

https://golangprojectstructure.com/how-to-make-go-structs-more-efficient/
https://go101.org/article/memory-layout.html

A tool exists, as noted in the first blog post link, that can find inefficient orderings of fields and provide the correct order.

https://cs.opensource.google/go/x/tools/+/refs/tags/v0.26.0:go/analysis/passes/fieldalignment/cmd/fieldalignment/main.go

$ ~/go/bin/fieldalignment ./...

I ran this tool on all the code in go-libp2p and fixed the 4 highest offenders.
Before:

/home/ubuntu/kazzmir-go-libp2p/p2p/host/resource-manager/scope.go:33:20: size is 160 but could be 80 diff=80
/home/ubuntu/kazzmir-go-libp2p/p2p/net/connmgr/connmgr.go:29:19: size is 2240 but could be 2152 diff=88
/home/ubuntu/kazzmir-go-libp2p/p2p/host/resource-manager/extapi.go:30:26: size is 120 but could be 24 diff=96
/home/ubuntu/kazzmir-go-libp2p/p2p/host/resource-manager/limit_defaults.go:349:25: size is 616 but could be 40 diff=576
/home/ubuntu/kazzmir-go-libp2p/p2p/host/resource-manager/limit_defaults.go:514:26: size is 616 but could be 40 diff=576
/home/ubuntu/kazzmir-go-libp2p/p2p/http/auth/internal/handshake/server.go:86:32: size is 1384 but could be 312 diff=1072
/home/ubuntu/kazzmir-go-libp2p/p2p/host/resource-manager/limit_defaults.go:24:25: size is 1192 but could be 40 diff=1152

After:

/home/ubuntu/kazzmir-go-libp2p/p2p/host/resource-manager/scope.go:33:20: size is 160 but could be 80 diff=80
/home/ubuntu/kazzmir-go-libp2p/p2p/net/connmgr/connmgr.go:29:19: size is 2240 but could be 2152 diff=88
/home/ubuntu/kazzmir-go-libp2p/p2p/host/resource-manager/extapi.go:30:26: size is 120 but could be 24 diff=96

Note that there are many more structs to fix, but the savings is moderate for them (under 100 bytes of savings for each struct).

@MarcoPolo
Copy link
Collaborator

This doesn't do what you think it does. Refer to this comment: golang/go#44877 (comment)

Note that diagnostic is about pointer bytes, not about size. That is, how many bytes of the object that the GC has to potentially scan for pointers.


this change is not harmful (unless you consider the ordering of fields useful for documentation purposes).

For some of these structs it is very useful to have related fields together for documentation purposes as you say.

The only change here that may be useful is moving the buf [1024]byte down in PeerIDAuthHandshakeServer. But these kinds of changes need to be accompanied by a benchmark that shows some change.

@kazzmir
Copy link
Author

kazzmir commented Oct 17, 2024

You are right about the pointer lines that the fieldalignment tool outputs, but I think these lines are truly about the size of the struct in memory?

/home/ubuntu/kazzmir-go-libp2p/p2p/protocol/autonatv2/pb/autonatv2.pb.go:334:22: struct of size 56 could be 48
/home/ubuntu/kazzmir-go-libp2p/p2p/protocol/autonatv2/pb/autonatv2.pb.go:389:19: struct of size 56 could be 48
/home/ubuntu/kazzmir-go-libp2p/p2p/protocol/autonatv2/pb/autonatv2.pb.go:546:23: struct of size 48 could be 40
/home/ubuntu/kazzmir-go-libp2p/p2p/protocol/identify/id.go:139:16: struct of size 304 could be 296
/home/ubuntu/kazzmir-go-libp2p/p2p/protocol/identify/opts.go:3:13: struct of size 64 could be 56
/home/ubuntu/kazzmir-go-libp2p/p2p/security/noise/session.go:18:20: struct of size 280 could be 272
/home/ubuntu/kazzmir-go-libp2p/p2p/transport/webrtc/connection.go:42:17: struct of size 200 could be 192
/home/ubuntu/kazzmir-go-libp2p/p2p/transport/webrtc/stream.go:67:13: struct of size 200 could be 176
/home/ubuntu/kazzmir-go-libp2p/p2p/transport/webtransport/transport.go:70:16: struct of size 184 could be 176
/home/ubuntu/kazzmir-go-libp2p/p2p/host/basic/basic_host.go:69:16: struct of size 464 could be 456
/home/ubuntu/kazzmir-go-libp2p/p2p/host/basic/basic_host.go:118:15: struct of size 208 could be 184
/home/ubuntu/kazzmir-go-libp2p/p2p/net/swarm/swarm.go:152:12: struct of size 512 could be 504
/home/ubuntu/kazzmir-go-libp2p/config/config.go:77:13: struct of size 568 could be 512
/home/ubuntu/kazzmir-go-libp2p/p2p/transport/tcp/metrics.go:204:18: struct of size 96 could be 88
/home/ubuntu/kazzmir-go-libp2p/p2p/transport/websocket/conn.go:20:11: struct of size 72 could be 64
/home/ubuntu/kazzmir-go-libp2p/p2p/transport/websocket/listener.go:23:15: struct of size 320 could be 312
/home/ubuntu/kazzmir-go-libp2p/p2p/http/libp2phttp.go:125:11: struct of size 176 could be 160
/home/ubuntu/kazzmir-go-libp2p/p2p/http/auth/server.go:18:23: struct of size 80 could be 72
/home/ubuntu/kazzmir-go-libp2p/core/crypto/fixture_test.go:17:15: struct of size 24 could be 16

Is it worthwhile to fix a few of the larger differences?

@MarcoPolo
Copy link
Collaborator

Sure, if the following conditions are true:

  1. It's not autogenerated code (like the pb.go files).
  2. It's a sizeable difference (at least over a cacheline in size). Bonus points if it's an object that is often allocated with many instances.
  3. You can show a measurable difference in before and after with a benchmark.

@MarcoPolo
Copy link
Collaborator

Closing to tidy up outstanding PRs. Feel free to reopen if you fulfill the above conditions.

@MarcoPolo MarcoPolo closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants