Add gsl::span vs std::span performance benchmark CI (fixes #1167)#1252
Open
Gafoor2005 wants to merge 14 commits into
Open
Add gsl::span vs std::span performance benchmark CI (fixes #1167)#1252Gafoor2005 wants to merge 14 commits into
Gafoor2005 wants to merge 14 commits into
Conversation
Add benchmark suite comparing gsl::span vs std::span performance across is_sorted and min_element operations. Includes GitHub Actions workflow for automated testing on GCC 14, Clang 18, and MSVC with results uploaded as artifacts for performance regression analysis.
Ensure both text and JSON benchmark outputs are properly populated in workflow artifacts by running benchmarks separately instead of using pipe redirection, which was leaving the text file empty.
Test PR for PR-based benchmark
Author
|
@microsoft-github-policy-service agree |
carsonRadtke
requested changes
May 29, 2026
Member
carsonRadtke
left a comment
carsonRadtke
left a comment
There was a problem hiding this comment.
Thanks for getting this put together! It'll be great to have the infrastructure necessary to compare GSL types to standard library types. I have briefly reviewed many of the changes, please address my comments and I will perform a more in-depth review once comments have been resolved. I will also assign Copilot as a reviewer, so you get some more feedback; please address those comments too.
This currently is a proof-of-concept of a really cool feature that will help us develop GSL, but I want to make sure the PR is clean and well-understood before running any workflows and merging your changes.
Also, using AI as a tool to write code is great. Please make sure you understand the code that you are contributing. I want to hold all contributions to a high standard and right now this PR needs some work.
| @@ -0,0 +1,413 @@ | |||
| # .github/workflows/span_benchmark.yml | |||
Member
There was a problem hiding this comment.
nit: probably don't need the filename here.
Comment on lines
+3
to
+20
| # Benchmarks gsl::span vs std::span on every PR across all supported compilers. | ||
| # Strategy: in-run ratio (gsl_ns / std_ns) — noise-resistant because both | ||
| # spans run in the same process, same machine, same moment. | ||
| # | ||
| # All dependencies (google-benchmark v1.9.0, GSL v4.1.0) are pulled via | ||
| # FetchContent inside the repo's CMakeLists.txt — no separate install steps. | ||
| # The build/_deps directory is cached, keyed on CMakeLists.txt hash, so | ||
| # repeated runs don't re-clone anything. | ||
| # | ||
| # Three separate job groups (one per OS) because `runs-on` can't share a | ||
| # matrix with OS-specific shell/path differences cleanly: | ||
| # | ||
| # benchmark-linux → ubuntu-latest → GCC-13, GCC-14, Clang-17, Clang-18 | ||
| # benchmark-windows → windows-latest → MSVC 2022, clang-cl | ||
| # benchmark-macos → macos-14 → Apple Clang (Xcode latest) | ||
| # | ||
| # A final `comment` job waits for all three groups, collects every JSON | ||
| # artifact, runs check_regression.py, and posts (or updates) one PR comment. |
Member
There was a problem hiding this comment.
nit: a brief description is okay, but this seems excessive.
| - label: GCC-13-cpp20 | ||
| cxx: g++-13 | ||
| cppstd: 20 | ||
| extra_flags: "" |
Member
There was a problem hiding this comment.
Looks like extra_flags is always empty. Can we remove it?
| cppstd: 23 | ||
| extra_flags: "" | ||
|
|
||
|
|
Member
There was a problem hiding this comment.
Seems like a lot of empty space. Can we clean this up?
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: |
Member
There was a problem hiding this comment.
I'm not familiar with matrix.include, but can't we just do something like:
matrix:
cxx: [g++-12, g++-13, g++-14, clang++-16, clang++-17, clang++-18]
cppstd: [20, 23]
Seems easier that enumerating every variation.
Comment on lines
+88
to
+96
| # ── Clang + -fbounds-safety (core motivation of issue #1167) ─── | ||
| # Safe Buffers enforcement is what triggered this whole tracking effort. | ||
| # - label: Clang-18-cpp20-bounds-safety | ||
| # cxx: clang++-18 | ||
| # cppstd: 20 | ||
| # extra_flags: "-fbounds-safety" | ||
| # ---------------------------------------------------------------------------- | ||
| # ⚠️ Note: As of early 2026, the feature is not yet fully available or stable in mainline, official releases (like Clang 20, 21, or 22). | ||
| # ---------------------------------------------------------------------------- |
Member
There was a problem hiding this comment.
I don't think -fbounds-safety is correct here.
Comment on lines
+105
to
+110
| - name: Install GCC 14 | ||
| if: startsWith(matrix.cxx, 'g++-14') | ||
| run: | | ||
| sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y | ||
| sudo apt-get update -qq | ||
| sudo apt-get install -y g++-14 |
Member
There was a problem hiding this comment.
We don't install gcc in the regular compilers workflow. Why do we need to do it here?
| @@ -0,0 +1,144 @@ | |||
| # `gsl::span` vs `std::span` Benchmark | |||
Member
There was a problem hiding this comment.
We can delete this file, I think.
| @@ -0,0 +1,87 @@ | |||
| # benchmark/CMakeLists.txt | |||
Member
There was a problem hiding this comment.
Some general notes:
- I'd like this to be very similar to GSL_TEST; can we have a no
-DGSL_BENCHMARKwhen invoking the root CMakeLists.txt? - I also would like to rely on the same
GSL_CXX_STANDARD? - We already download googletest for the test suite, is it necessary to do it again here? They are also different versions. Why?
- Google benchmark latest tag is 1.9.5, can we update to that?
- There is a performance cost to
-fno-omit-frame-pointer, can we not enable the omission?
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a CI-run benchmark suite to continuously track performance parity between gsl::span and std::span, generating a per-PR Markdown report and failing CI when gsl::span exceeds a configured slowdown threshold.
Changes:
- Adds a Google Benchmark-based executable comparing
gsl::spanvsstd::spanacross several workloads. - Adds a Python regression checker that parses benchmark JSON, computes
gsl/stdratios, and emits a PR-comment-ready Markdown report. - Adds a GitHub Actions workflow that runs the benchmark across a multi-OS/compiler/standard matrix, uploads results, and posts/updates a single PR comment.
Show a summary per file
| File | Description |
|---|---|
| benchmark/span_bench.cpp | Adds the benchmark program comparing gsl::span and std::span workloads. |
| benchmark/README.md | Documents purpose, local usage, CI matrix, and how regression detection works. |
| benchmark/CMakeLists.txt | Self-contained CMake build that fetches benchmark dependencies and builds against the local GSL checkout. |
| benchmark/check_regression.py | Parses JSON results, pairs Std/GSL benchmarks, computes ratios, and generates Markdown + CI status. |
| .github/workflows/span_benchmark.yml | CI matrix runner that builds/runs benchmarks, aggregates artifacts, and posts/updates a PR comment. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 10
Comment on lines
+5
to
+7
| #include "gsl/span" | ||
| #include <benchmark/benchmark.h> | ||
| #include <numeric> |
Comment on lines
+202
to
+217
| config_regression = False | ||
| for p in pairs: | ||
| ratio, status = ratio_and_status(p["gsl_mean"], p["std_mean"], threshold) | ||
| if "regression" in status: | ||
| had_regression = True | ||
| config_regression = True | ||
|
|
||
| lines.append( | ||
| f"| `{p['short']}` " | ||
| f"| {fmt(p['std_mean'])} " | ||
| f"| {fmt_stddev(p['std_stddev'], p['std_mean'])} " | ||
| f"| {fmt(p['gsl_mean'])} " | ||
| f"| {fmt_stddev(p['gsl_stddev'], p['gsl_mean'])} " | ||
| f"| {ratio:.2f}× " | ||
| f"| {status} |" | ||
| ) |
Comment on lines
+4
to
+8
| # All dependencies are fetched automatically via FetchContent: | ||
| # - google/benchmark v1.9.0 | ||
| # - google/googletest release-1.12.1 (benchmark's internal dep) | ||
| # - microsoft/GSL v4.1.0 | ||
| # |
Comment on lines
+7
to
+10
| # All dependencies (google-benchmark v1.9.0, GSL v4.1.0) are pulled via | ||
| # FetchContent inside the repo's CMakeLists.txt — no separate install steps. | ||
| # The build/_deps directory is cached, keyed on CMakeLists.txt hash, so | ||
| # repeated runs don't re-clone anything. |
| uses: actions/cache@v4 | ||
| with: | ||
| path: build/_deps | ||
| key: fetchcontent-linux-${{ matrix.cxx }}-${{ hashFiles('CMakeLists.txt') }} |
| uses: actions/cache@v4 | ||
| with: | ||
| path: build/_deps | ||
| key: fetchcontent-windows-${{ matrix.toolset }}-${{ hashFiles('CMakeLists.txt') }} |
| uses: actions/cache@v4 | ||
| with: | ||
| path: build/_deps | ||
| key: fetchcontent-macos-${{ matrix.cppstd }}-${{ hashFiles('CMakeLists.txt') }} |
| python3 benchmark/check_regression.py \ | ||
| --threshold 0.15 \ | ||
| --output comment.md \ | ||
| --workflow-url "https://github.com/${{ github.repository }}/blob/tree/main/.github/workflows/span_benchmark.yml" \ |
Comment on lines
+389
to
+395
| - name: Find existing bot comment | ||
| uses: peter-evans/find-comment@v3 | ||
| id: find_comment | ||
| with: | ||
| issue-number: ${{ github.event.pull_request.number }} | ||
| comment-author: github-actions[bot] | ||
| body-includes: "<!-- span-bench-report -->" |
Comment on lines
+399
to
+405
| - name: Create or update PR comment | ||
| uses: peter-evans/create-or-update-comment@v4 | ||
| with: | ||
| comment-id: ${{ steps.find_comment.outputs.comment-id }} | ||
| issue-number: ${{ github.event.pull_request.number }} | ||
| body-path: comment.md | ||
| edit-mode: replace |
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
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
Adds a continuous benchmark suite that tracks performance parity between
gsl::spanandstd::spanon every PR, across all supported compilersand platforms.
Closes #1167.
What this adds
benchmark/span_bench.cpp— benchmarks from @galenelias comparinggsl::spanandstd::spanacross five workloads (is_sorted, ranges,custom iterator loop, min_element algorithm, range-for min)
benchmark/CMakeLists.txt— self-contained build; fetchesgoogle-benchmark v1.9.0 and googletest via FetchContent; links against
the local repo's GSL (not a pinned tag) so every PR tests its own
changes
benchmark/check_regression.py— parses benchmark JSON output, pairsStdSpan/GslSpan variants by name, computes
gsl_ns / std_nsratio,writes a Markdown table, exits 1 on regression
.github/workflows/span_benchmark.yml— runs on every PR across 13configs (GCC 13/14, Clang 17/18, MSVC 2022, clang-cl, Apple Clang ×
C++20/23); posts or updates a single PR comment with results from all
configs
Approach
The comparison is in-run ratio (
gsl_ns / std_ns) rather thancomparing against a stored baseline. Both span variants run in the same
process on the same machine at the same moment, so GitHub-hosted runner
noise (~10–15%) cancels out. A ratio above 1.15 (15% slower than
std::span) fails CI.This sidesteps the "Status: Blocked" concern about noisy shared runners —
no self-hosted runner or external service needed.
Each benchmark runs 10 repetitions; the script uses the mean and
reports stddev so reviewers can see measurement stability.
CI matrix
Example PR comment
The workflow posts (and updates) a single comment per PR that looks like:
Notes
-fbounds-safetyconfig is stubbed out in the matrix with a commentexplaining it's not yet stable in mainline Clang releases — easy to
uncomment when it lands
CMakeLists.txthash so CI doesn'tre-clone on every run
peter-evans/find-comment+create-or-update-commentso repeated pushes to the same PR updatethe existing comment rather than flooding the thread