Skip to content

Change expectedSymbols to 2048#7398

Merged
SungJin1212 merged 4 commits into
cortexproject:masterfrom
SungJin1212:Change-expectedSymbols
Apr 7, 2026
Merged

Change expectedSymbols to 2048#7398
SungJin1212 merged 4 commits into
cortexproject:masterfrom
SungJin1212:Change-expectedSymbols

Conversation

@SungJin1212
Copy link
Copy Markdown
Member

This PR increases expectedSymbols to 2048 to reduce slice reallocations. The current value of 20 is very small in the production level.
As shown in the attached chart, after applying the change to 2048 (around 19:30), I have observed reduced allocation spikes.
image

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Copy link
Copy Markdown
Member

@CharlieTLe CharlieTLe left a comment

Choose a reason for hiding this comment

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

Thanks, I wonder why it was set to 20 initially.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Apr 6, 2026
Comment thread CHANGELOG.md Outdated
# Changelog

## master / unreleased
* [CHANGE] Distributor: Increase the default capacity of `expectedSymbols` from 20 to 2048 in the PRW2 path to reduce slice reallocations. #7397
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* [CHANGE] Distributor: Increase the default capacity of `expectedSymbols` from 20 to 2048 in the PRW2 path to reduce slice reallocations. #7397
* [CHANGE] Distributor: Increase the default capacity of `expectedSymbols` from 20 to 2048 in the PRW2 path to reduce slice reallocations. #7398

Comment thread pkg/cortexpb/timeseriesv2.go Outdated

var (
expectedSymbols = 20
expectedSymbols = 2048
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ultimately, this is about labels. if your requests have more labels, you will need this.

what do you think about something like this?

var symbolsHighWaterMark atomic.Int64

  func init() {
        symbolsHighWaterMark.Store(int64(expectedSymbols))
  }

  func ReuseWriteRequestV2(req *PreallocWriteRequestV2) {
        // Update high watermark, capped to prevent outliers from inflating all allocations.
        if n := int64(cap(req.Symbols)); n > symbolsHighWaterMark.Load() && n <= 8192 {
                symbolsHighWaterMark.Store(n)
        }
        // ... existing cleanup code ...
  }

  writeRequestPoolV2 = sync.Pool{
        New: func() any {
                return &PreallocWriteRequestV2{
                        WriteRequestV2: WriteRequestV2{
                                Symbols: make([]string, 0, symbolsHighWaterMark.Load()),
                        },
                }
        },
  }

Starts at expectedSymbols (20), grows to match real usage, caps at 8192 (128KB per pooled object) to prevent runaway allocation from outlier requests.

Copy link
Copy Markdown
Member Author

@SungJin1212 SungJin1212 Apr 7, 2026

Choose a reason for hiding this comment

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

Sound goods, I introduced an adaptive way. Can you take a look?
I think the initial 20 is small, so set it to 128.

Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
@pull-request-size pull-request-size Bot added size/L and removed size/XS labels Apr 7, 2026
@SungJin1212
Copy link
Copy Markdown
Member Author

@CharlieTLe
I think when I introduced PRW2 initially, I was unable to set the value carefully.

Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Copy link
Copy Markdown
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

LGTM

@SungJin1212 SungJin1212 merged commit d38e2a4 into cortexproject:master Apr 7, 2026
91 of 95 checks passed
@SungJin1212 SungJin1212 mentioned this pull request Apr 7, 2026
3 tasks
friedrichg added a commit that referenced this pull request Apr 9, 2026
The existing benchmark compared dynamic EMA against fixed-128 with
unequal cleanup: the dynamic path called full ReuseWriteRequestV2
(Source reset, Timeseries handling, EMA update) while the fixed path
only cleared Symbols. This biased timing in favor of the fixed path.

Changes:
- Equalize cleanup in fixed_capacity to match ReuseWriteRequestV2
  (minus the EMA update)
- Add fixed_capacity_2048 sub-benchmark to compare against the
  original #7398 proposal, answering whether EMA complexity is
  justified over a well-tuned static value

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>
friedrichg added a commit that referenced this pull request Apr 9, 2026
The existing benchmark compared dynamic EMA against fixed-128 with
unequal cleanup: the dynamic path called full ReuseWriteRequestV2
(Source reset, Timeseries handling, EMA update) while the fixed path
only cleared Symbols. This biased timing in favor of the fixed path.

Changes:
- Equalize cleanup in fixed_capacity to match ReuseWriteRequestV2
  (minus the EMA update)
- Add fixed_capacity_2048 sub-benchmark to compare against the
  original #7398 proposal, answering whether EMA complexity is
  justified over a well-tuned static value

Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>
friedrichg added a commit that referenced this pull request Apr 14, 2026
…ymbol pooling comparison (#7412)

* Add a benchmark to compare the fixed symbol with the dynamic

Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>

* Equalize benchmark cleanup and add fixed_capacity_2048 sub-benchmark

The existing benchmark compared dynamic EMA against fixed-128 with
unequal cleanup: the dynamic path called full ReuseWriteRequestV2
(Source reset, Timeseries handling, EMA update) while the fixed path
only cleared Symbols. This biased timing in favor of the fixed path.

Changes:
- Equalize cleanup in fixed_capacity to match ReuseWriteRequestV2
  (minus the EMA update)
- Add fixed_capacity_2048 sub-benchmark to compare against the
  original #7398 proposal, answering whether EMA complexity is
  justified over a well-tuned static value

Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>

---------

Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>
Co-authored-by: SungJin1212 <tjdwls1201@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size/L type/performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants