Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions .github/workflows/span_benchmark.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
name: Span Benchmark

on:
pull_request:
branches: [main]

# Cancel stale runs when a new commit is pushed to the same PR.
concurrency:
group: span-bench-${{ github.event.pull_request.number }}
cancel-in-progress: true

jobs:
benchmark-linux:
name: Linux / ${{ format('{0}-cpp{1}', matrix.cxx, matrix.cppstd) }}
runs-on: ubuntu-latest
strategy:
fail-fast: false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

matrix:
cxx: [g++-13, g++-14, clang++-16, clang++-17, clang++-18]
cppstd: [20, 23]
exclude:
- cxx: g++-13
cppstd: 23
- cxx: clang++-17
cppstd: 23
Comment on lines +21 to +25

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining why these are excluded would be helpful for future readers (and me presently).

steps:
- uses: actions/checkout@v4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make sure we are using the latest actions? I think actions/checkout is at v7 and actions/cache is at v5 and actions/upload-artifact is also at v7. (Please check any others I may have missed).


- name: Cache FetchContent dependencies
uses: actions/cache@v4
with:
path: build/_deps
key: fetchcontent-linux-${{ matrix.cxx }}-${{ hashFiles('benchmark/CMakeLists.txt') }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't cppstd matter?

restore-keys: |
fetchcontent-linux-${{ matrix.cxx }}-

- name: Configure

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "Configure", "Build", and "Run benchmark", can we repurpose the existing ./.github/workflows/cmake workflow?

run: |
cmake -S . -B build \
-DGSL_BENCHMARK=ON \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_CXX_COMPILER=${{ matrix.cxx }} \
-DCMAKE_CXX_STANDARD=${{ matrix.cppstd }} \

- name: Build
run: cmake --build build --target span_bench -j$(nproc)

- name: Run benchmark
run: |
./build/span_bench \
--benchmark_format=json \
--benchmark_repetitions=10 \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did we arrive at 10 being sufficient?

--benchmark_report_aggregates_only=true \
--benchmark_out=results_${{ format('{0}-cpp{1}', matrix.cxx, matrix.cppstd) }}.json

- name: Upload results
uses: actions/upload-artifact@v4
with:
name: bench-${{ format('{0}-cpp{1}', matrix.cxx, matrix.cppstd) }}
path: results_${{ format('{0}-cpp{1}', matrix.cxx, matrix.cppstd) }}.json
retention-days: 7

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for 7 days?


benchmark-windows:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also apply my linux comments to windows and macos.

name: Windows / ${{ format('{0}-cpp{1}', matrix.toolset || 'MSVC', matrix.cppstd) }}
runs-on: windows-latest
strategy:
fail-fast: false
matrix:
generator: [ 'Visual Studio 17 2022' ]
toolset: [ '', 'ClangCL' ]
cppstd: [ 20, 23 ]
steps:
- uses: actions/checkout@v4

- name: Cache FetchContent dependencies
uses: actions/cache@v4
with:
path: build/_deps
key: fetchcontent-windows-${{ matrix.toolset }}-${{ hashFiles('benchmark/CMakeLists.txt') }}
restore-keys: |
fetchcontent-windows-${{ matrix.toolset }}-

- name: Configure
shell: pwsh
run: |
$tsArg = if ("${{ matrix.toolset }}" -ne "") { @("-T", "${{ matrix.toolset }}") } else { @() }

cmake -S . -B build `
-DGSL_BENCHMARK=ON `
-G "${{ matrix.generator }}" @tsArg `
-DCMAKE_CXX_STANDARD=${{ matrix.cppstd }} `

- name: Build
shell: pwsh
run: |
cmake --build build --target span_bench `
--config Release -j $env:NUMBER_OF_PROCESSORS

- name: Run benchmark
shell: pwsh
run: |
.\build\Release\span_bench.exe `
--benchmark_format=json `
--benchmark_repetitions=10 `
--benchmark_report_aggregates_only=true `
--benchmark_out=results_${{ format('{0}-cpp{1}', matrix.toolset || 'MSVC', matrix.cppstd) }}.json

- name: Upload results
uses: actions/upload-artifact@v4
with:
name: bench-${{ format('{0}-cpp{1}', matrix.toolset || 'MSVC', matrix.cppstd) }}
path: results_${{ format('{0}-cpp{1}', matrix.toolset || 'MSVC', matrix.cppstd) }}.json
retention-days: 7

benchmark-macos:
name: macOS / ${{ format('AppleClang-cpp{0}', matrix.cppstd) }}
runs-on: macos-latest
strategy:
fail-fast: false
matrix:
cppstd: [ 20, 23 ]
steps:
- uses: actions/checkout@v4

- name: Cache FetchContent dependencies
uses: actions/cache@v4
with:
path: build/_deps
key: fetchcontent-macos-${{ matrix.cppstd }}-${{ hashFiles('benchmark/CMakeLists.txt') }}
restore-keys: |
fetchcontent-macos-${{ matrix.cppstd }}-

- name: Configure
run: |
cmake -S . -B build \
-DGSL_BENCHMARK=ON \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_CXX_STANDARD=${{ matrix.cppstd }} \

- name: Build
run: cmake --build build --target span_bench -j$(sysctl -n hw.logicalcpu)

- name: Run benchmark
run: |
./build/span_bench \
--benchmark_format=json \
--benchmark_repetitions=10 \
--benchmark_report_aggregates_only=true \
--benchmark_out=results_${{ format('AppleClang-cpp{0}', matrix.cppstd) }}.json

- name: Upload results
uses: actions/upload-artifact@v4
with:
name: bench-${{ format('AppleClang-cpp{0}', matrix.cppstd) }}
path: results_${{ format('AppleClang-cpp{0}', matrix.cppstd) }}.json
retention-days: 7
76 changes: 76 additions & 0 deletions .github/workflows/span_benchmark_comment.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
name: Span Benchmark Comment

on:
workflow_run:
workflows: ["Span Benchmark"]
types: [completed]

permissions:
pull-requests: write

jobs:
comment:
name: Post PR comment
runs-on: ubuntu-latest
if: always()

steps:
- uses: actions/checkout@v6

# Download all bench-* artifacts from the triggering workflow run.
- name: Download all result artifacts
uses: actions/download-artifact@v4
with:
pattern: bench-*
merge-multiple: true
github-token: ${{ secrets.GITHUB_TOKEN }}
run-id: ${{ github.event.workflow_run.id }}

- name: Generate regression report
id: report
run: |
python3 benchmark/check_regression.py \
--threshold 0.15 \
--output comment.md \
results_*.json
continue-on-error: true

# Resolve the PR number from the triggering workflow run.
- name: Get PR number
id: pr
uses: actions/github-script@v7
with:
script: |
const runs = await github.rest.pulls.list({
owner: context.repo.owner,
repo: context.repo.repo,
state: 'open',
});
const pr = runs.data.find(p =>
p.head.sha === context.payload.workflow_run.head_sha
);
return pr ? pr.number : null;

- name: Find existing bot comment
if: steps.pr.outputs.result != 'null'
uses: peter-evans/find-comment@v3
id: find_comment
with:
issue-number: ${{ steps.pr.outputs.result }}
comment-author: github-actions[bot]
body-includes: "<!-- span-bench-report -->"

- name: Create or update PR comment
if: steps.pr.outputs.result != 'null'
uses: peter-evans/create-or-update-comment@v4
with:
comment-id: ${{ steps.find_comment.outputs.comment-id }}
issue-number: ${{ steps.pr.outputs.result }}
body-path: comment.md
edit-mode: replace

- name: Fail if regression detected
if: steps.report.outcome == 'failure'
run: |
echo "::error::Performance regression detected. See the PR comment for the full table."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: technically the cause could also have been bad arguments passed to check_regression.py.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check the return code? I think argparse will exit with code 2 for bad arguments and check_regression.py will exit with code 1 for a regression.

exit 1
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ string(COMPARE EQUAL ${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_SOURCE_DIR} PROJECT_IS_

option(GSL_INSTALL "Generate and install GSL target" ${PROJECT_IS_TOP_LEVEL})
option(GSL_TEST "Build and perform GSL tests" ${PROJECT_IS_TOP_LEVEL})
option(GSL_BENCHMARK "Build span benchmarks" OFF)

# The implementation generally assumes a platform that implements C++14 support
target_compile_features(GSL INTERFACE "cxx_std_14")
Expand All @@ -19,6 +20,10 @@ add_subdirectory(include)

target_sources(GSL INTERFACE $<BUILD_INTERFACE:${GSL_SOURCE_DIR}/GSL.natvis>)

if(GSL_BENCHMARK)
add_subdirectory(benchmark)
endif()
Comment thread
Gafoor2005 marked this conversation as resolved.

if (GSL_TEST)
enable_testing()
add_subdirectory(tests)
Expand Down
57 changes: 57 additions & 0 deletions benchmark/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
cmake_minimum_required(VERSION 3.14...3.16)

project(GSLBenchmarks LANGUAGES CXX)

# ── C++ standard ───────────────────────────────────────────────────────────────
# Minimum is 20 because std::span (required for this benchmark) is C++20 only.
# If a consumer passes GSL_CXX_STANDARD < 20 we clamp and warn.
set(GSL_CXX_STANDARD "20" CACHE STRING "Use c++ standard")

if(GSL_CXX_STANDARD LESS 20)
message(WARNING
"GSL_CXX_STANDARD=${GSL_CXX_STANDARD} is too old for the span benchmark "
"(std::span requires C++20). Overriding to 20.")
set(GSL_CXX_STANDARD "20" CACHE STRING "Use c++ standard" FORCE)
endif()
Comment on lines +10 to +15

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of a warning (and ignoring the user's standard version), I'd rather see the test just get skipped. In the future we may want to benchmark against other GSL features so the check should probably move down to add_executable(span_bench span_bench.cpp) business.


set(CMAKE_CXX_STANDARD ${GSL_CXX_STANDARD})
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# Makes Visual Studio organise targets into folders
set_property(GLOBAL PROPERTY USE_FOLDERS ON)

include(FetchContent)

# Suppress benchmark's own test suite
set(BENCHMARK_ENABLE_TESTING OFF CACHE BOOL "" FORCE)
set(BENCHMARK_ENABLE_INSTALL OFF CACHE BOOL "" FORCE)

FetchContent_Declare(
googlebenchmark
GIT_REPOSITORY https://github.com/google/benchmark.git
GIT_TAG v1.9.5
GIT_SHALLOW ON
)
FetchContent_MakeAvailable(googlebenchmark)

add_executable(span_bench span_bench.cpp)

target_link_libraries(span_bench PRIVATE
benchmark::benchmark
Microsoft.GSL::GSL
)

set_target_properties(span_bench PROPERTIES FOLDER "benchmarks")

# ── Optimisation flags ─────────────────────────────────────────────────────────
target_compile_options(span_bench PRIVATE
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:
-O3
-march=native
>
$<$<CXX_COMPILER_ID:MSVC>:
/O2
/GL
>
)
Loading