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

Reuse buffer with sync.Pool #350

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Conversation

kramaranya
Copy link
Contributor

While investigating a task #334 to consider sync.Pool in our code, I discovered that currently, each invocation of templateWithValue creates a new bytes.Buffer and discards it. By switching to a sync.Pool to reuse and reset these buffers, we can cut down on allocations and reduce overhead.

Here are the benchmark results I gathered using benchstat to compare before_change.txt (no sync.Pool) versus after_change.txt (using sync.Pool):

name                old time/op    new time/op    delta
TemplateWithValue-12    171.5ns ± 4%   145.4ns ± 0%  -15.19% (p=0.000 n=10)

name                old alloc/op   new alloc/op   delta
TemplateWithValue-12    192B ± 0%     80B ± 0%    -58.33% (p=0.000 n=10)

name                old allocs/op  new allocs/op  delta
TemplateWithValue-12      6.0 ± 0%     4.0 ± 0%   -33.33% (p=0.000 n=10)

Reusing the buffer with sync.Pool is better because it lowers overhead from creating new buffers on each call, reduces memory usage and helps the garbage collector by cutting down on short-lived objects:

  • Time per operation improved by about 15%.
  • Memory usage dropped by more than 50%.
  • Number of allocations reduced from 6 to 4.

@@ -147,11 +148,20 @@ func (r *TemplatedResourceAttributesGenerator) Generate(ctx context.Context, att
return attrs
}

var bufPool = sync.Pool{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I am a burned child as I worked with JavaScript in the crazy days, but usually you avoid having a "global" variable.
As it is local to the package, it might be okaish, if we would rename it to something like "templateBufferPool".

A better solution would be to initiate this in the constructor and pass it down from the struct into the templateWithValue function. According to the docs, we must not copy it, so we would need to use a pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've addressed your suggestion

@stlaz stlaz merged commit 544589f into brancz:sig-auth-acceptance Jan 22, 2025
7 checks passed
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.

3 participants