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

Improved, configurable buffer pools #87

Merged
merged 2 commits into from
Apr 20, 2021
Merged

Improved, configurable buffer pools #87

merged 2 commits into from
Apr 20, 2021

Conversation

nsmith5
Copy link
Contributor

@nsmith5 nsmith5 commented Apr 15, 2021

Problem

As discussed in #86 and #85, the current buffer pool leads to heap allocations that are 64 * the largest template rendered. This wastes memory. We cannot simply remove the buffer pool though because it is needed to check template errors before writing HTTP status codes.

Solution

Add a sized buffer pool implementation that garbage collects buffers that grow past a certain point. Use this sized buffer pool by default with some heuristically choosen defaults (I've selected 32 buffers of size 512KiB), but allow users to specify their own if needed. A GenericBufferPool interface has been added that allows users to not only modify the SizedBufferPool and BufferPool implementations, but to implement their own if need be.

Results

Two benchmarks have been added: BenchmarkBigHTMLBuffers and BenchmarkSmallHTMLBuffers where the buffer pool buffers are larger and smaller than the rendered template respectively. As expected, the small buffers lead to more allocations and slower HTML templating than the larger buffers.

benchmark-diff

Upon inspecting the memory profiles for each benchmark we see the difference in allocation is precisely where we think it should be: buffers are being allocated upon return to the pool because they've out grown our size limit

Buffer larger than templates

big-buffer-profile

Buffer smaller than templates

small-buffer-profile

Copy link
Owner

@unrolled unrolled left a comment

Choose a reason for hiding this comment

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

This is awesome! Just one tiny comment to fix up. Thanks for tackling this issue 👍

@nsmith5
Copy link
Contributor Author

nsmith5 commented Apr 19, 2021

Which comment @unrolled? I didn't see any comments on the PR just yet.

render.go Outdated Show resolved Hide resolved
@unrolled unrolled merged commit d232abd into unrolled:v1 Apr 20, 2021
@unrolled
Copy link
Owner

Thanks!!

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