Skip to content

Chunk recoder optimization#297

Merged
vporoshok merged 11 commits into
ppfrom
chunk_recoder_optimization
Apr 29, 2026
Merged

Chunk recoder optimization#297
vporoshok merged 11 commits into
ppfrom
chunk_recoder_optimization

Conversation

@cherep58
Copy link
Copy Markdown
Collaborator

// Before optimization
BenchmarkChunkRecoder_min 869701624 ns

// After optimization
BenchmarkChunkRecoder_min 745188236 ns

Boost: ~14%

@cherep58 cherep58 requested a review from gshigin April 21, 2026 12:12
@cherep58 cherep58 requested a review from vporoshok as a code owner April 21, 2026 12:12
@cherep58 cherep58 self-assigned this Apr 23, 2026
vporoshok

This comment was marked as outdated.

@vporoshok vporoshok dismissed their stale review April 28, 2026 11:38

Re-publishing in English.

Copy link
Copy Markdown
Collaborator

@vporoshok vporoshok 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 optimization — the benchmark numbers look nice. However, there are several serious concerns plus a handful of minor ones; see inline comments.

Blockers (must be addressed before merge):

  • UB through std::variant in UniversalDecodeIterator::operator* / operator->reinterpret_cast over the variant's storage relies on stdlib implementation details. The static_assert below only checks sample_'s offset within each alternative, not the layout of std::variant itself.
  • UniversalDecodeIterator::set — same issue but with a write; the method is not even used in this PR.

Important:

  • This PR adds but does not use: seek_to, invalidate_sample, set, SeekResult::kUpdateSampleNextAndStop, UniversalDecodeIterator::Type. Better to introduce them in the PR that actually needs them.
  • Typo Universale in the public concept name AssignableFromUniversaleDecodeIterator.
  • [[likely]] on the kUpdateSample branch in seek is misplaced — the hot path here is ChunkRecoder, which always returns kNext.
  • noexcept was dropped from UniversalDecodeIterator::operator== / operator++ — no throwing handler involved there.

Nice to have:

  • The LE assumption in bstream.h::write_byte (at minimum, add static_assert(std::endian::native == std::endian::little), or align the style with the existing BareBones::Bit::be(...) usage).
  • varint_buffer without {}-initialization in xor.h — needs a contract comment.
  • [[likely]] on two consecutive branches in xor.h.
  • std::ignore = timestamp_decoder_.decode(); in traits.h.
  • The public default constructor of UniversalDecodeIterator creates a "dummy" state.
  • The PR description should explain which technique contributes how much — in particular, whether bypassing std::variant is actually required for the +14%.
  • Nit: missing trailing newline in traits.h.

Comment thread pp/series_data/decoder/universal_decode_iterator.h
Comment thread pp/series_data/decoder/universal_decode_iterator.h
Comment thread pp/series_data/decoder/universal_decode_iterator.h Outdated
Comment thread pp/series_data/decoder/universal_decode_iterator.h Outdated
Comment thread pp/series_data/decoder/universal_decode_iterator.h Outdated
Comment thread pp/series_data/decoder/traits.h
Comment thread pp/prometheus/tsdb/chunkenc/bstream.h
Comment thread pp/prometheus/tsdb/chunkenc/xor.h
Comment thread pp/prometheus/tsdb/chunkenc/xor.h Outdated
Comment thread pp/head/chunk_recoder.h Outdated
@vporoshok vporoshok merged commit 4cdb385 into pp Apr 29, 2026
54 of 59 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.

2 participants