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

Improvements to buffer pool #466

Merged
merged 1 commit into from
Feb 28, 2023
Merged

Improvements to buffer pool #466

merged 1 commit into from
Feb 28, 2023

Conversation

lthibault
Copy link
Collaborator

@lthibault lthibault commented Feb 25, 2023

  • Removes unused buffer pool buckets
  • Improves buffer reuse by consuming buffers even if capacity is not aligned to bucket boundary
  • Aggressively reuses small-sized buffers by setting min page size to 1kb
  • Improves buffer reuse by decreasing total number of buckets
  • Small readability improvements (use range instead of classic for loop)
  • Increase max page size to 1Mb

Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

A couple things in-line

message.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
message.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

One more minor style nit

exp/bufferpool/pool.go Outdated Show resolved Hide resolved
@lthibault
Copy link
Collaborator Author

I ended up re-writing a significant portion of this to allow users to provide their own pool implementations. I think this can have a very large impact in niche cases (e.g. embedded systems), where you might want to tune the pooling behavior to save memory. I can also imagine applications that allocate a large volume of very big segments.

I've also added tests around the sensitive parts of the default pool implementation.

capability.go Outdated Show resolved Hide resolved
@lthibault
Copy link
Collaborator Author

Weird, I'm getting build failures. Re-running 🤨

This was referenced Feb 25, 2023
@lthibault
Copy link
Collaborator Author

Ok fixed. Ready for review.

Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

Some comments in-line

exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Show resolved Hide resolved
exp/bufferpool/pool.go Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
exp/bufferpool/pool.go Outdated Show resolved Hide resolved
zenhack
zenhack previously approved these changes Feb 27, 2023
@lthibault
Copy link
Collaborator Author

Ok, I think this is finally ready to merge. How do you feel about squashing this branch?

@zenhack
Copy link
Contributor

zenhack commented Feb 28, 2023

Once we figure out CI, SGTM.

@lthibault lthibault merged commit 29a3330 into main Feb 28, 2023
@lthibault lthibault deleted the cleanup/fix-bufpool branch February 28, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants