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

MultiSegmentArena: Fix release back into bufferpool #598

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

matheusd
Copy link
Contributor

@matheusd matheusd commented Jan 7, 2025

This fixes a bug that was preventing correct release of buffers allocated by Decode() back to the global buffer pool.

Previously, the data variable was overriden on the loop that splits the segments, preventing the correct tracking of the full data buffer and its subsequent return back to the buffer pool when the arena was released.

This also adds a .Release() call to the Decode benchmark to better reflect the ability to reuse buffers.

This fixes a bug that was preventing correct release of buffers
allocated by Decode() back to the global buffer pool.

Previously, the data variable was overriden on the loop that splits the
segments, preventing the correct tracking of the full data buffer and
its subsequent return back to the buffer pool when the arena was
released.

This also adds a .Release() call to the Decode benchmark to better
reflect the ability to reuse buffers.
@lthibault
Copy link
Collaborator

@fpetkovski Just want to say "thank you" for your reviews. 🙏

@matheusd Aiming to have this reviewed in the next few days.

@matheusd
Copy link
Contributor Author

matheusd commented Jan 9, 2025

Cool, thanks!

I have another small improvement that depends on this, to be pushed after this one is merged.

Copy link
Contributor

@Semisol Semisol left a comment

Choose a reason for hiding this comment

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

LGTM, 2.4x speedup

goos: darwin
goarch: amd64
cpu: Intel(R) Xeon(R) W-2140B CPU @ 3.20GHz

BenchmarkDecode-16 [master]  130
  9046418 ns/op  106.12 MB/s
  5244827  B/op   50087 allocs/op
BenchmarkDecode-16 [fix-msa] 309
  3732257 ns/op  257.22 MB/s
   802463  B/op   10004 allocs/op

Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

Thanks for the benchmarks @Semisol. This is a big perf win! I feel like we should be communicating publicly about this kind of improvement. Great work @matheusd!

@lthibault lthibault merged commit f9295d9 into capnproto:main Jan 10, 2025
4 checks passed
@Semisol
Copy link
Contributor

Semisol commented Jan 10, 2025

Thanks for the benchmarks @Semisol. This is a big perf win! I feel like we should be communicating publicly about this kind of improvement. Great work @matheusd!

I think some documentation should be added, as the issue here was partly misusing the API. We need better documentation overall :)

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.

4 participants