Replace strconv.Quote with strconv.AppendQuote everywhere to reduce allocations#426
Open
asergunov wants to merge 4 commits intokubernetes:mainfrom
Open
Replace strconv.Quote with strconv.AppendQuote everywhere to reduce allocations#426asergunov wants to merge 4 commits intokubernetes:mainfrom
strconv.Quote with strconv.AppendQuote everywhere to reduce allocations#426asergunov wants to merge 4 commits intokubernetes:mainfrom
Conversation
Updated the logging functions to utilize cached buffers more effectively by replacing the creation of new byte slices with calls to `AvailableBuffer()`. This change reduces memory allocations and enhances performance across multiple logging components, including `klog`, `klogr_slog`, and `textlogger`. Additionally, modified the JSON serialization in `keyvalues_slog.go` and `keyvalues.go` to leverage the same buffer optimization for quoting string values. This refactor aims to streamline logging operations and improve overall efficiency. Signed-off-by: Anton Sergunov <anton.sergunov@flant.com>
Signed-off-by: Anton Sergunov <anton.sergunov@flant.com>
Signed-off-by: Anton Sergunov <anton.sergunov@flant.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asergunov The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-triage
|
Welcome @asergunov! |
k8s-ci-robot
added
cncf-cla: no
Author
|
Don't take in account the reported speed improvements. It could be just Laptop heating. It has less allocations for sure. |
pohly
reviewed
Nov 21, 2025
pohly
left a comment
pohly
left a comment
There was a problem hiding this comment.
I double-checked performance on an Arrow Lake desktop with pinning to one P-core (to avoid variance). I see moderate improvements:
pkg: k8s.io/klog/v2
...
geomean 61.43n 61.20n -0.37%
pkg: k8s.io/klog/v2/textlogger
...
geomean 586.6n 578.7n -1.35%
But any improvement which doesn't unduly increase complexity is welcome, so thanks for the PR!
@asergunov: can you squash into one commit?
/assign @dims
Tests passed locally, workflows still need to be triggered (don't have permissions for that).
Member
|
i've approved workflows to run @pohly |
Author
|
@pohly could you please give a performance test on this commit There is the one replacing also the 1024 bytes buffer allocation on the stack. I mean writing to the bufer itself directly should be a bit faster than writing to stack and then copying to the buffer. |
|
@asergunov: here's a comparison before and with 0f1b22b |
Author
|
@pohly Thanks a lot! What do you think which one we should merge? |
Author
|
I mean I can see a benefit in removing hardcoded magic value and removing extra copy. Are these results acceptable? |
|
The PR looks reasonable to me and we should merge it. But I don't understand the question about "which one we should merge"? |
Author
I mean the latest one or at this commit 0f1b22b The latest state is not changing the two places when we use 1024 bytes buffer on stack |
Author
|
Should we also update the 1024 bytes buffer stack allocations with direct buffer writes? |
|
So the question is whether these changes are wanted: https://github.com/kubernetes/klog/compare/0f1b22b..5025e083b0c56 5025e08 diff cleanup The 6db40fd commit makes the code more complex again (on-stack 1024 bytes buffer). It claims to reduce the number of allocations (right?) but I don't see that when using Earlier, I misunderstood what you wanted to be benchmarked. In #426 (comment) I compared 0f1b22b against its predecessor, not the full branch. Here's the 0f1b22b..5025e08 performance comparison. It's actually a bit worse. |
Member
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
@asergunov: can you clarify? Should I see less allocations because of that commit? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Spotted calls to
strconv.Quotewhich allocates the new string. Replaced withstrconv.AppendQuote.In most of the places I'm using the pattern:
So we reusing the already allocated buffer if it's available.
The only exception is stack-allocated fixed size buffers we have introduced in #413. As you can see from the
benchmark_out_reduce-allocations-fix-alloc.txtcolumn it's faster in benchmarks as it is. Thebenchmark_out_reduce-allocations.txtis a results with these places also replaced and you can see it performs worse in some cases. So I kept that code untouched.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: