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

Improve memory alignment #673

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve memory alignment #673

wants to merge 1 commit into from

Conversation

mrueg
Copy link
Contributor

@mrueg mrueg commented Jan 10, 2024

I run https://github.com/dkorunic/betteralign against the codebase and it showed the following improvements for memory alignment

    bbolt/db.go:38:9: struct of size 528 could be 496
    bbolt/db.go:1012:12: struct with 40 pointer bytes could be 24
    bbolt/db.go:1280:14: struct of size 104 could be 80
    bbolt/freelist.go:24:15: struct with 136 pointer bytes could be 112
    bbolt/node.go:12:11: struct with 88 pointer bytes could be 72
    bbolt/tx.go:27:9: struct with 192 pointer bytes could be 88
    bbolt/internal/common/inode.go:8:12: struct with 48 pointer bytes could be 32
    bbolt/internal/common/page.go:324:15: struct with 16 pointer bytes could be 8
    bbolt/cmd/bbolt/main.go:445:22: struct with 16 pointer bytes could be 8
    bbolt/cmd/bbolt/main.go:1489:19: struct with 160 pointer bytes could be 104
    bbolt/cmd/bbolt/main.go:1546:16: struct with 24 pointer bytes could be 16
    bbolt/internal/btesting/btesting.go:28:9: struct with 48 pointer bytes could be 40
    bbolt/tests/dmflakey/dmflakey.go:133:13: struct with 64 pointer bytes could be 56

@mrueg mrueg force-pushed the betteralgin branch 2 times, most recently from fef18ee to f9ce17b Compare January 10, 2024 23:32
db.go Outdated
//nolint
dataref []byte // mmap'ed readonly, write throws SEGV
txs []*Tx

// Put `stats` at the first field to ensure it's 64-bit aligned. Note that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please read this comment on the alignment issues we had with some arch (ARM?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch, thanks!

bbolt/db.go:38:9: struct with 336 pointer bytes could be 192

I guess still an improvement over 528 bytes

Copy link
Contributor

@tjungblu tjungblu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

is anyone able to validate the arm builds?

bbolt/db.go:38:9: struct of size 528 could be 496
bbolt/db.go:1012:12: struct with 40 pointer bytes could be 24
bbolt/db.go:1280:14: struct of size 104 could be 80
bbolt/freelist.go:24:15: struct with 136 pointer bytes could be 112
bbolt/node.go:12:11: struct with 88 pointer bytes could be 72
bbolt/tx.go:27:9: struct with 192 pointer bytes could be 88
bbolt/internal/common/inode.go:8:12: struct with 48 pointer bytes could be 32
bbolt/internal/common/page.go:324:15: struct with 16 pointer bytes could be 8
bbolt/cmd/bbolt/main.go:445:22: struct with 16 pointer bytes could be 8
bbolt/cmd/bbolt/main.go:1489:19: struct with 160 pointer bytes could be 104
bbolt/cmd/bbolt/main.go:1546:16: struct with 24 pointer bytes could be 16
bbolt/internal/btesting/btesting.go:28:9: struct with 48 pointer bytes could be 40
bbolt/tests/dmflakey/dmflakey.go:133:13: struct with 64 pointer bytes could be 56

Signed-off-by: Manuel Rüger <[email protected]>
@ahrtr
Copy link
Member

ahrtr commented Jan 12, 2024

Thanks for the improvement.

  • For the DB struct: Let's keep it unchanged. All unexported fields should be put at the bottom. The stats Stats is just a special case.
  • For the BenchOptions , flakey and Tx: Suggest to keep them unchanged. It makes more sense to group the parameters together as they are.

@ahrtr
Copy link
Member

ahrtr commented Feb 3, 2024

Please let me know if you have bandwidth to resolve my above comment #673 (comment)

@mrueg
Copy link
Contributor Author

mrueg commented Feb 8, 2024

Please let me know if you have bandwidth to resolve my above comment #673 (comment)

Thanks! I should have more bandwidth in the upcoming weeks.
Before this gets merged, I would want to check #691 and see if the change is actually improving anything.

tjungblu added a commit to tjungblu/bbolt that referenced this pull request Jun 7, 2024
This runs betteralign to pack structs smarter.
Originally proposed by etcd-io#673

Co-authored-by: Manuel Rüger <[email protected]>
Signed-off-by: Thomas Jungblut <[email protected]>
tjungblu added a commit to tjungblu/bbolt that referenced this pull request Jun 7, 2024
Originally proposed by etcd-io#673

Co-authored-by: Manuel Rüger <[email protected]>
Signed-off-by: Thomas Jungblut <[email protected]>
tjungblu added a commit to tjungblu/bbolt that referenced this pull request Jun 7, 2024
This runs betteralign to pack structs smarter.
Originally proposed by etcd-io#673

Co-authored-by: Manuel Rüger <[email protected]>
Signed-off-by: Thomas Jungblut <[email protected]>
@tjungblu
Copy link
Contributor

tjungblu commented Jun 7, 2024

@mrueg if you don't mind, I'm currently test running this with @ahrtr's suggestions applied in my own fork with openshift/etcd#278 - will report any e2e improvement I'm seeing.

I think the benchmark tooling will need a little more time :)

@tjungblu
Copy link
Contributor

tjungblu commented Jun 24, 2024

Just to leave some quick feedback here. I had some trouble with Go 1.22 and our build pipelines, so my test basically just consisted of running the installation routines (aka an IDLE cluster).

There was no measurable improvement in memory usage or apiserver/etcd latency, I would only expect that under higher load though. That was also a bad apples/oranges test, because etcd 3.5 uses a different version of bbolt than what we're using here. I'll do another test run when I find some more time.

tjungblu added a commit to tjungblu/bbolt that referenced this pull request Jul 8, 2024
This runs betteralign to pack structs smarter.
Originally proposed by etcd-io#673

Co-authored-by: Manuel Rüger <[email protected]>
Signed-off-by: Thomas Jungblut <[email protected]>
@github-actions github-actions bot added the stale label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants