Uh oh!
There was an error while loading. Please reload this page.
perf: optimize classic-only histogram observe path - #2252
Open
arnabnandy7 wants to merge 13 commits into
Open
Conversation
arnabnandy7
requested review from
dhoard, fstab, jaydeluca and zeitlinger
as code ownersJune 24, 2026 19:40
arnabnandy7force-pushed
the
perf/synchronized-histogram
branch
from
June 25, 2026 20:57
6eaf8a0 to
8f7d299CompareSigned-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
arnabnandy7force-pushed
the
perf/synchronized-histogram
branch
from
June 25, 2026 21:07
8f7d299 to
591d6adCompareContributor
|
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlingerforce-pushed
the
perf/synchronized-histogram
branch
from
July 2, 2026 14:42
2dea810 to
e539344CompareSigned-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlinger added a commit
that referenced
this pull request
Jul 7, 2026
## Summary Extract the PR benchmark workflows from #2252 into a standalone PR so they can land on `main` first. ## Why The benchmark job uploads a Markdown artifact with the PR comment contents, but the actual PR comment is posted by a separate `workflow_run` workflow. GitHub only triggers `workflow_run` workflows when the workflow file already exists on the default branch, so keeping the reporter in the same PR prevents it from commenting on that PR's benchmark run. Splitting these workflows out first lets subsequent benchmark PRs use the comment-reporting flow as intended. ## Changes - add `PR Benchmarks` workflow triggered by the `benchmark` label on pull requests - add `PR Benchmark Report` workflow triggered by `workflow_run` - keep the benchmark summary as an uploaded artifact consumed by the reporter workflow ## Testing - `mise run lint:fix`
1 task
zeitlinger added a commit
that referenced
this pull request
Jul 8, 2026
## Summary The `PR Benchmark Report` workflow failed to comment on [#2252](#2252) after a successful benchmark run ([run 28865892835](https://github.com/prometheus/client_java/actions/runs/28865892835)): ``` gh: Not Found (HTTP 404) parse "https://api.github.com/repos/prometheus/client_java/issues/comments/{\r": net/url: invalid control character in URL ``` Root cause: `workflow_run.pull_requests` is empty when the triggering run's head branch is not on the default branch (or the PR is from a fork). With `PR_NUMBER` empty, the subsequent `repos/${REPO}/issues//comments` API call 404s and the paginator emits a malformed URL, so no comment gets posted. ## Changes - Fall back to `repos/{owner}/{repo}/commits/{sha}/pulls` to resolve the PR from the head SHA when `workflow_run.pull_requests` is empty. - Fail fast with an explicit error if the PR still can't be resolved, so future regressions don't cascade into cryptic URL-parse errors. ## Test plan - [ ] Re-run the report workflow against run 28861406560 (or a fresh push to #2252) and confirm a benchmark comment lands on the PR. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlinger added a commit
that referenced
this pull request
Jul 9, 2026
## Summary Fix the `PR Benchmark Report` workflow for forked PR benchmark runs like #2252, where `workflow_run.pull_requests` and `commits/{sha}/pulls` can both be empty for the upstream repository. ## Changes - Resolve the PR number from the uploaded benchmark artifact names first. - Fall back to the workflow run head repository owner and branch via `repos/{owner}/{repo}/pulls`. - Keep the existing commit-SHA lookup as a final fallback. - Remove the artifact ZIP download `Accept: application/octet-stream` header because `gh api` rejected it during local replay. ## Validation - Replayed the resolution logic against benchmark run `28928458470` and confirmed it resolves PR #2252 and downloads artifact `8164318312`. - Ran `mise run lint:fix`. - Ran `git diff --check`.
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
|
zeitlinger added a commit
that referenced
this pull request
Jul 10, 2026
## Summary Make PR benchmark comments answer whether the PR is faster or slower than its base commit, while keeping wall-clock time close to a single benchmark run. ## Changes - Run base and head benchmarks in parallel matrix jobs. - Add a lightweight summary job that downloads both raw JMH artifacts and generates the PR comment. - Add a `Comparison with base` table to the generated `README.md` / PR comment. - Mark each benchmark as `faster`, `slower`, or `within noise` using JMH confidence intervals when available. - Capture and show runner hardware for both base and head jobs, including a note that parallel jobs may run on different hardware. - Rename the existing per-run relative column to `Within run` so it is clear that it is not the base comparison. - Store `baseline-results.json` in the uploaded benchmark artifact for inspection. ## Validation - Ran `mise run lint:fix`. - Ran `python3 -m py_compile .mise/tasks/generate_benchmark_summary.py`. - Ran `git diff --check`. - Replayed summary generation with the latest #2252 benchmark artifact and the published benchmark baseline; the output now shows `HistogramBenchmark.prometheusClassic` as `-78.6%` / `slower`. - Verified no-baseline summary generation still omits the comparison section. ## Tradeoff Parallel base/head jobs are faster wall-clock than measuring both commits sequentially on one runner, but runner hardware can differ. The comment now prints both runner descriptions and calls out that caveat.
zeitlinger
commented
Jul 10, 2026
Member
the bench is not comparing to main - I'll fix that |
arnabnandy7
commented
Jul 10, 2026
ContributorAuthor
thanks |
zeitlinger added a commit
that referenced
this pull request
Jul 20, 2026
## Summary Replace the parallel base/head PR benchmark comparison with topic-split benchmark jobs. Each topic compares base and head sequentially on the same runner, while topics still run in parallel for better wall-clock time. ## Changes - Split PR benchmarks into topic jobs: counter, histogram, and exposition. - Run base and head sequentially inside each topic job so each comparison uses the same runner. - Merge per-topic JMH JSON results in the summary job before generating the PR comment. - Add a comparison note clarifying that base/head are compared on the same runner within each topic. - Add histogram benchmark variants for classic-only histograms: - shared contended 4-thread benchmark remains as `prometheusClassic` - single-thread benchmark: `prometheusClassicSingleThread` - per-thread 4-thread benchmark: `prometheusClassicPerThread` ## Why Running base and head in separate jobs was faster but not reliable enough because GitHub Actions can schedule them on different CPU models. Topic splitting keeps the reliable same-runner comparison within each topic while still allowing unrelated benchmark topics to run in parallel. ## Validation - Ran `mise run lint:fix`. - Ran `mise run build`. - Ran `python3 -m py_compile .mise/tasks/generate_benchmark_summary.py`. - Ran `git diff --check`. - Verified the JMH include patterns select the intended benchmarks with `java -jar benchmarks/target/benchmarks.jar -l ...`. - Smoke-tested the topic result merge and summary generation using the latest #2252 benchmark artifact.
zeitlinger
requested changes
Jul 23, 2026
zeitlinger
left a comment
Member
There was a problem hiding this comment.
The shared-observer benchmark regression is blocking for this implementation: serializing every classic-only observation on the DataPoint monitor removes the contention-avoiding behavior we need on this path.
I opened #2329 as an experimental alternate candidate so we can compare an epoch-striped/double-buffered per-thread accumulator against main on the same benchmark runner. Its local four-thread JMH smoke result was 40,944.515 ± 302.666 ops/s, but that is not a controlled cross-branch result and should not be treated as proof. The full benchmark workflow for #2329 is queued at https://github.com/prometheus/client_java/actions/runs/29990012023.
The alternate is not production-ready yet. Before it could be considered for merge, it still needs:
- Java Memory Model review and preferably dedicated jcstress coverage for epoch handoff races;
- a decision on bounding/reclaiming per-thread cells under high thread churn;
- scrape latency and stalled-observer spin-wait measurements;
- evaluation of the epoch-flip/cell-scan cost in
getCount()andgetSum(); - the full CI benchmark comparison and broader supported-JDK/architecture coverage.
The benchmark can be rerun by removing and re-adding the benchmark label on the alternate PR.
I recommend keeping #2252 blocked while we collect the alternate benchmark and decide whether the striped design can be hardened, rather than merging the global-monitor regression.
zeitlinger
commented
Jul 23, 2026
Member
Follow-up: the alternate draft's current head is |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary
Optimizes
Histogram.classicOnly()observations by adding a dedicated classic-only fast path that uses synchronized plain fields instead of the native/hybridLongAdder/DoubleAdderplus buffer path.Fixes#1915
Changes
Histogram.DataPoint.observe()andobserveWithExemplar()through the classic-only fast path whennativeInitialSchema == CLASSIC_HISTOGRAM.Testing
.\mvnw compile -pl prometheus-metrics-core -Dcoverage.skip=true -Dcheckstyle.skip=truegit diff --check