perf: experiment with striped classic histogram accumulator - #2329
Open
zeitlinger wants to merge 11 commits into
Open
perf: experiment with striped classic histogram accumulator#2329zeitlinger wants to merge 11 commits into
zeitlinger wants to merge 11 commits into
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Contributor
|
1 similar comment
Contributor
|
Contributor
Benchmark resultsBenchmark run succeeded for
Prometheus Java Client BenchmarksRun Information
Comparison with base
Results for PR headCounterBenchmark
HistogramBenchmark
HistogramTextFormatBenchmark
TextFormatUtilBenchmark
Raw ResultsNotes
Benchmark Descriptions
|
Member
Author
|
Evaluation (local, JDK 25.0.4, x86_64, 1 fork / 2 warmups / 5 measurements; indicative rather than production benchmark):
Recommendation: use #2329 as the basis, not #2252, but do not merge yet. Please add jcstress/JMM coverage, bound or reclaim retained cells, and define bounded scrape progress/latency when a writer is delayed. Existing focused concurrency tests pass repeatedly, but they do not close those risks. |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Member
Author
|
Hardening update: pushed
Focused accumulator/Histogram tests, repeated concurrency tests, benchmark compilation, and formatter/lint pass locally. Full CI benchmark matrix and cross-JDK/architecture validation remain appropriate before merge. |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Member
Author
|
Follow-up hardening pushed in
Local focused tests: 5 accumulator tests and 35 Histogram tests pass, with the accumulator suite repeated five times. JMH smoke (JDK 25.0.4, x86_64, 1 fork, 1×100 ms warmup, 2×100 ms measurement, 1,000 short-lived recorders; setup excluded): collect 1,218 ops/s and getCount 1,600 ops/s. These are indicative scan-cost measurements only. The current |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Member
Author
|
JCStress evidence added in
The stalled-writer test uses reflection only in the JCStress test module to model a pause at the internal epoch boundary; production code has no test hooks. Cross-JDK/architecture stress coverage remains for CI/maintainer validation. |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlinger
marked this pull request as ready for review
August 19, 2026 08:30
zeitlinger
added a commit
that referenced
this pull request
Aug 19, 2026
## Summary - Add an explicit base-vs-head regression verdict using comparable JMH metadata, confidence-interval overlap, and a practical 5% threshold. - Remove cross-method `Nx slower` rankings from the PR-head tables. - List head-only benchmarks separately as descriptive results with no regression verdict. ## Motivation The [#2329 benchmark report](#2329 (comment)) described `HistogramBenchmark.openTelemetryExponential` as “20x slower” only because it was ranked against a different benchmark method in the same run. Its actual base/head delta was -5.7% with overlapping confidence intervals (within noise). This change makes that distinction explicit and prevents the within-run ranking from being mistaken for a regression. ## Tests - `python3 .mise/tasks/test_generate-benchmark-summary.py` - `python3 .mise/tasks/test_update-benchmarks.py` - `ruff check` and `ruff format --check` on the changed Python files - `mise run lint` (full lint is blocked only by the pre-existing README Slack link returning HTTP 403; the pre-push scoped lint passed) Related: [#2329](#2329). --------- Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
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
1 participant
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.
Summary
This is an experimental alternate implementation for benchmarking against
mainand the synchronized candidate in #2252. It is not being presented as production-ready yet.For classic-only histograms, this replaces the existing
Buffer/LongAdderrecording path with an epoch-striped, double-buffered per-thread accumulator. Each recording thread owns a cell, observations update plain thread-confined bucket/count/sum fields, and collection advances a global epoch before draining the inactive buffers. This avoids a globalsynchronized (DataPoint)monitor on the shared four-thread observe path while preserving coherent bucket/count/sum snapshots.getCount()andgetSum()use the same snapshot mechanism.Native-only and hybrid histograms keep their existing implementation. The public API is unchanged.
Local evidence
mise run lint./mvnw -pl prometheus-metrics-core -Dtest=HistogramTest test— 35 tests passed./mvnw -pl prometheus-metrics-core -am verify— reactor passed; core ran 157 testsgit diff --checkHistogramBenchmark.prometheusClassic: 40,944.515 ± 302.666 ops/sThe local JMH number is a smoke result, not a controlled cross-branch conclusion. The
benchmarkworkflow is being requested so this branch andmainare measured on the same runner.Added concurrency coverage
getCount()/getSum()valuesclear()lifecycle behavior for old and newly-created data pointsRemaining hardening before merge consideration
getCount()/getSum(), which currently perform an epoch flip and cell scan to return exact accumulated values.Related: #2252