perf: optimize classic-only histogram observe path#2252
Open
arnabnandy7 wants to merge 12 commits into
Open
Conversation
arnabnandy7
requested review from
dhoard,
fstab,
jaydeluca and
zeitlinger
as code owners
arnabnandy7
force-pushed
the
perf/synchronized-histogram
branch
from
6eaf8a0 to
8f7d299
Compare
Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
arnabnandy7
force-pushed
the
perf/synchronized-histogram
branch
from
8f7d299 to
591d6ad
Compare
Contributor
|
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlinger
force-pushed
the
perf/synchronized-histogram
branch
from
2dea810 to
e539344
Compare
Signed-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.
Member
|
the bench is not comparing to main - I'll fix that |
Contributor
Author
thanks |
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
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