Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add benchmarks #103

Merged
merged 81 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 78 commits
Commits
Show all changes
81 commits
Select commit Hold shift + click to select a range
bbbd892
Start adding benchmarks
ptoffy Jan 9, 2025
b79b3d7
Remove approval requirement for now
ptoffy Jan 9, 2025
5046e43
Update workflow
ptoffy Jan 9, 2025
b74536c
Update workflow
ptoffy Jan 9, 2025
05422f5
Add more benchmarks
ptoffy Jan 9, 2025
48bd331
Update benchmark CI with the jwt-kit one
MahdiBM Jan 10, 2025
0445266
[CI] ci to get triggered on every file change at in Benchmarks folder
MahdiBM Jan 10, 2025
c291cdd
Try RunsOn magic cache
MahdiBM Jan 10, 2025
8f15f7a
configure thresholds
MahdiBM Jan 10, 2025
8efd726
remove s3-cache from this specific ci file
MahdiBM Jan 10, 2025
79286b9
Create runs-on.yml
MahdiBM Jan 10, 2025
e35811c
Merge branch 'main' into benchmarks
MahdiBM Jan 10, 2025
b48c22d
update thresholds
MahdiBM Jan 10, 2025
972d8af
move runs-on configuration step
MahdiBM Jan 10, 2025
0125fcf
get thresholds working
MahdiBM Jan 10, 2025
f421467
update machine name
MahdiBM Jan 10, 2025
a7ce239
Update machine name
MahdiBM Jan 10, 2025
214145e
use `cpuUser` instead of the other cpu metrics
MahdiBM Jan 11, 2025
cd0230b
update benchmarks
MahdiBM Jan 11, 2025
6c5a948
Fix benchmark code
MahdiBM Jan 11, 2025
9024661
rename machine
MahdiBM Jan 11, 2025
86b00c7
configure RunsOn first
MahdiBM Jan 11, 2025
ad4c1c7
adjust benchmarks
MahdiBM Jan 11, 2025
8dbe4f1
fix threshold files names
MahdiBM Jan 11, 2025
663ce3b
do 1000x for cpu time
MahdiBM Jan 11, 2025
f998b9c
fix ranges
MahdiBM Jan 11, 2025
b0d7f16
revert some of the iteration changes
MahdiBM Jan 11, 2025
a367d13
aesthetical rename
MahdiBM Jan 11, 2025
6558319
Try better CPU benchmarks
MahdiBM Jan 11, 2025
900304c
use 1000 scale factor for cpu benchmarks
MahdiBM Jan 11, 2025
f44bbc0
try 100x with manual
MahdiBM Jan 11, 2025
6ab382d
refinements
MahdiBM Jan 12, 2025
a1b111a
add tolerances
MahdiBM Jan 12, 2025
43906e4
fix tolerance
MahdiBM Jan 12, 2025
432eec0
SerializerCPUTime 10x -> 100x
MahdiBM Jan 12, 2025
7ec4449
quote branch names for clarity
MahdiBM Jan 12, 2025
66a211b
update thresholds
MahdiBM Jan 12, 2025
4a51cfb
resolve nit picks @gwynne is going to make
MahdiBM Jan 12, 2025
f3ff558
increase iterations for more stability
MahdiBM Jan 12, 2025
8185d07
increase max duration
MahdiBM Jan 12, 2025
8f6f223
better thresholds and iterations
MahdiBM Jan 12, 2025
b691cde
more adjustments
MahdiBM Jan 12, 2025
d055d79
allocation benchmarks to only run once
MahdiBM Jan 12, 2025
e3197b0
increase big message size to what it should be
MahdiBM Jan 12, 2025
009f2e9
decrease rounds
MahdiBM Jan 12, 2025
74a0aea
minor refinements
MahdiBM Jan 12, 2025
9b748cc
try something weird
MahdiBM Jan 12, 2025
6128a4a
try different method
MahdiBM Jan 13, 2025
a89c638
Use `unsafeUninitializedCapacity` initialiser
ptoffy Jan 13, 2025
79dc04d
Revert "Use `unsafeUninitializedCapacity` initialiser"
ptoffy Jan 13, 2025
7528237
Try something
MahdiBM Jan 13, 2025
0ec09f1
minor fixes
MahdiBM Jan 13, 2025
9b30045
minor
MahdiBM Jan 13, 2025
4c4cc77
refinements
MahdiBM Jan 13, 2025
3259d8c
smaller chunk
MahdiBM Jan 13, 2025
dcb061a
use array
MahdiBM Jan 13, 2025
e946cb5
more fixes
MahdiBM Jan 13, 2025
b23f6c7
minor fixes
MahdiBM Jan 13, 2025
c6d84ad
refinements
MahdiBM Jan 13, 2025
c9ac81b
just use AsyncStream
MahdiBM Jan 13, 2025
8bfa20b
better thresholds
MahdiBM Jan 13, 2025
263c36c
more benchs + revert to `AsyncSyncStream` for max accuracy
MahdiBM Jan 13, 2025
33efdfd
refinements
MahdiBM Jan 13, 2025
b71aceb
more benchmarks and refinements
MahdiBM Jan 13, 2025
bc3c03a
fixes
MahdiBM Jan 13, 2025
b5d2aa6
adjust thresholds
MahdiBM Jan 13, 2025
668bb5d
minor refinement
MahdiBM Jan 13, 2025
2a95328
thresholds
MahdiBM Jan 13, 2025
e2f37e5
thresholds
MahdiBM Jan 13, 2025
3599f31
thresholds
MahdiBM Jan 13, 2025
3378157
increase iterations
MahdiBM Jan 13, 2025
d634658
thresholds
MahdiBM Jan 14, 2025
7db849a
thresholds
MahdiBM Jan 14, 2025
464d97e
minor
MahdiBM Jan 14, 2025
9b7ef8d
better message
MahdiBM Jan 14, 2025
1651b63
thresholds
MahdiBM Jan 14, 2025
6a706b9
minor
MahdiBM Jan 14, 2025
82cba28
fix
MahdiBM Jan 14, 2025
da6eea1
better detect PR events
MahdiBM Jan 14, 2025
bcea652
Try thollander/actions-comment-pull-request
MahdiBM Jan 14, 2025
5619d5d
minor
MahdiBM Jan 14, 2025
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
264 changes: 264 additions & 0 deletions .github/workflows/benchmark.yml
Copy link
Member

Choose a reason for hiding this comment

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

Not right now but I'm surprised there aren't existing actions for commenting on issues and we should look at turning this into its own action (or submitting it to the swift lang repo)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually there are and we were using it but someone decided to do it by hand 😆 vapor/jwt-kit#222 (comment)

Copy link
Contributor

@MahdiBM MahdiBM Jan 14, 2025

Choose a reason for hiding this comment

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

The github-scripts doesn't really help, we'll end up with around the same amount of lines of code, just in js instead of bash.
Most of the code here is to keep track of the old comment and don't create new comments.
I think Tim is talking about a dedicated action that takes some parameters and just does the work. Something like actions/checkout@v4 but for this purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we had that before, that's what I meant with my comment in JWTKit https://github.com/thollander/actions-comment-pull-request but not sure if this can update comments too rather than creating new ones

Copy link
Contributor

Choose a reason for hiding this comment

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

it appears it can update comments too? I'd use it if it can.

In the original CI i wrote this CI file for, i can't do that since the comment keeps some more state with it as well, but here i should be able to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice https://github.com/thollander/actions-comment-pull-request seems to be working well.
It apparently works the same way my curl stuff were working, but it clears 80 lines out of the CI file so better to use this instead.

Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
name: benchmark
on:
workflow_dispatch:
pull_request_review:
types: [submitted]
pull_request:
branches: [main]
types: [synchronize]
paths:
- Sources/*.swift
- Benchmarks/
- .github/workflows/benchmark.yml

jobs:
benchmark-vs-thresholds:
# Run the job only if it's a manual workflow dispatch, or if this event is a pull-request approval event, or if someone has rerun the job.
if: github.event_name == 'workflow_dispatch' || github.event.review.state == 'approved' || github.run_attempt > 1

# https://runs-on.com/features/custom-runners/
runs-on:
labels:
- runs-on
- runner=2cpu-4ram
- run-id=${{ github.run_id }}

container: swift:noble

defaults:
run:
shell: bash

env:
PR_COMMENT: null # will be populated later

steps:
- name: Configure RunsOn
uses: runs-on/action@v1

- name: Check out code
uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Configure git
run: git config --global --add safe.directory "${GITHUB_WORKSPACE}"

# jemalloc is a dependency of the Benchmarking package
# actions/cache will detect zstd and will become much faster.
- name: Install jemalloc, curl, jq and zstd
run: |
set -eu

apt-get update -y
apt-get install -y libjemalloc-dev curl jq zstd

- name: Restore .build
id: restore-cache
uses: actions/cache/restore@v4
with:
path: Benchmarks/.build
key: "swiftpm-benchmark-build-${{ runner.os }}-${{ github.event.pull_request.base.sha || github.event.after }}"
restore-keys: "swiftpm-benchmark-build-${{ runner.os }}-"

- name: Run benchmarks for branch '${{ github.head_ref || github.ref_name }}'
run: |
swift package -c release --disable-sandbox \
--package-path Benchmarks \
benchmark baseline update \
'${{ github.head_ref || github.ref_name }}'

- name: Read benchmark result
id: read-benchmark
run: |
set -eu

swift package -c release --disable-sandbox \
--package-path Benchmarks \
benchmark baseline read \
'${{ github.head_ref || github.ref_name }}' \
--no-progress \
--format markdown \
>> result.text

# Read the result to the output of the step
echo 'result<<EOF' >> "${GITHUB_OUTPUT}"
cat result.text >> "${GITHUB_OUTPUT}"
echo 'EOF' >> "${GITHUB_OUTPUT}"

- name: Compare branch '${{ github.head_ref || github.ref_name }}' against thresholds
id: compare-benchmark
run: |
set -eu

TIMESTAMP=$(date -u +"%Y-%m-%d %H:%M:%S UTC")
ENCODED_TIMESTAMP=$(date -u +"%Y-%m-%dT%H%%3A%M%%3A%SZ")
TIMESTAMP_LINK="https://www.timeanddate.com/worldclock/fixedtime.html?iso=$ENCODED_TIMESTAMP"
echo "## Benchmark check running at [$TIMESTAMP]($TIMESTAMP_LINK)" >> summary.text

# Disable 'set -e' to prevent the script from exiting on non-zero exit codes
set +e
swift package -c release --disable-sandbox \
--package-path Benchmarks \
benchmark thresholds check \
'${{ github.head_ref || github.ref_name }}' \
--path "$PWD/Benchmarks/Thresholds/" \
--no-progress \
--format markdown \
>> summary.text
echo "exit-status=$?" >> "${GITHUB_OUTPUT}"
set -e

echo 'summary<<EOF' >> "${GITHUB_OUTPUT}"
cat summary.text >> "${GITHUB_OUTPUT}"
echo 'EOF' >> "${GITHUB_OUTPUT}"

- name: Cache .build
if: steps.restore-cache.outputs.cache-hit != 'true'
uses: actions/cache/save@v4
with:
path: Benchmarks/.build
key: "swiftpm-benchmark-build-${{ runner.os }}-${{ github.event.pull_request.base.sha || github.event.after }}"

- name: Construct comment
run: |
set -eu

EXIT_CODE='${{ steps.compare-benchmark.outputs.exit-status }}'

echo 'PR_COMMENT<<EOF' >> "${GITHUB_ENV}"

# The fact that the comment starts with <!-- benchmark ci tag. exit code: ' is used
# in a other steps to find this comment again.
# Be wary of that when changing this line.
echo "<!-- benchmark ci tag. exit code: $EXIT_CODE -->" >> "${GITHUB_ENV}"

echo '' >> "${GITHUB_ENV}"

echo '## [Benchmark](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) Report' >> "${GITHUB_ENV}"

case "${EXIT_CODE}" in
0)
echo '**✅ Pull request has no significant performance differences ✅**' >> "${GITHUB_ENV}"
;;
1)
echo '**❌ Pull request has significant performance differences 📊**' >> "${GITHUB_ENV}"
;;
2)
echo '**❌ Pull request has significant performance regressions 📉**' >> "${GITHUB_ENV}"
;;
4)
echo '**❌ Pull request has significant performance improvements 📈**' >> "${GITHUB_ENV}"
;;
*)
echo '**❌ Benchmark comparison failed to complete properly with exit code $EXIT_CODE ❌**' >> "${GITHUB_ENV}"
;;
esac

echo '<details>' >> "${GITHUB_ENV}"
echo ' <summary> Click to expand comparison result </summary>' >> "${GITHUB_ENV}"
echo '' >> "${GITHUB_ENV}"
echo '${{ steps.compare-benchmark.outputs.summary }}' >> "${GITHUB_ENV}"
echo '' >> "${GITHUB_ENV}"
echo '</details>' >> "${GITHUB_ENV}"

echo '' >> "${GITHUB_ENV}"

echo '<details>' >> "${GITHUB_ENV}"
echo ' <summary> Click to expand benchmark result </summary>' >> "${GITHUB_ENV}"
echo '' >> "${GITHUB_ENV}"
echo '${{ steps.read-benchmark.outputs.result }}' >> "${GITHUB_ENV}"
echo '' >> "${GITHUB_ENV}"
echo '</details>' >> "${GITHUB_ENV}"

echo 'EOF' >> "${GITHUB_ENV}"

- name: Output the comment as job summary
run: echo '${{ env.PR_COMMENT }}' >> "${GITHUB_STEP_SUMMARY}"

# There is a '<!-- benchmark ci tag. exit code: {some_number} -->' comment at the beginning of the benchamrk report comment.
# The number in that comment is the exit code of the last benchmark run.
- name: Find existing comment ID
if: github.event_name == 'pull_request'
id: existing-comment
run: |
set -eu

# Known limitation: This only fetches the first 100 comments. This should not
# matter much because a benchmark comment should have been sent early in the PR.
curl -sL \
-X GET \
-H 'Accept: application/vnd.github+json' \
-H 'Authorization: BEARER ${{ secrets.GITHUB_TOKEN }}' \
-H 'X-GitHub-Api-Version: 2022-11-28' \
-o result.json \
'https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.number }}/comments?per_page=100'

# Get the last comment that has a body that starts with '<!-- benchmark ci tag. exit code: '.
# If available, we can just update this comment instead of creating new ones.
EXISTING_COMMENT=$(
cat result.json |
jq '
[
.[] |
select(.body | startswith("<!-- benchmark ci tag. exit code: "))
][-1]
'
)

# Check if EXISTING_COMMENT is empty or null
if [ "$EXISTING_COMMENT" = "null" ] || [ -z "$EXISTING_COMMENT" ]; then
ID=""
BODY=""
PARSED_EXIT_CODE=""
else
ID="$(echo "$EXISTING_COMMENT" | jq '.id')"
BODY="$(echo "$EXISTING_COMMENT" | jq -r '.body')"
PARSED_EXIT_CODE="$(echo "$BODY" | head -n 1 | grep -o 'exit code: [0-9]\+' | grep -o '[0-9]\+')"

if [ "$ID" = "null" ]; then
echo "Comment ID was null? something is wrong"
echo "Comment: $EXISTING_COMMENT"
exit 111
fi
fi

# Output the results to GITHUB_OUTPUT
{
echo "id=$ID"
echo "exit-code=$PARSED_EXIT_CODE"
echo "body<<EOF"
echo "$BODY"
echo "EOF"
} >> "${GITHUB_OUTPUT}"

- name: Comment in PR
if: github.event_name == 'pull_request'
run: |
set -eu

EXISTING_COMMENT_ID="${{ steps.existing-comment.outputs.id }}"
# Need to provide the argument in a way that anything in the comment itself is not evaluated.
BODY_JSON="$(jq -n -c --arg COMMENT "$(echo '${{ env.PR_COMMENT }}')" '{"body": $COMMENT}')"

if [ -n "$EXISTING_COMMENT_ID" ]; then
echo "Will update a comment: $EXISTING_COMMENT_ID"
ENDPOINT="https://api.github.com/repos/${{ github.repository }}/issues/comments/$EXISTING_COMMENT_ID"
else
echo 'Will create a new comment'
ENDPOINT="https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.number }}/comments"
fi

curl -sL \
-X POST \
-H 'Accept: application/vnd.github+json' \
-H 'Authorization: BEARER ${{ secrets.GITHUB_TOKEN }}' \
-H 'X-GitHub-Api-Version: 2022-11-28' \
-d "$BODY_JSON" \
"$ENDPOINT"

- name: Exit with correct status
run: |
EXIT_CODE='${{ steps.compare-benchmark.outputs.exit-status }}'
echo "Previous exit code was: $EXIT_CODE"
exit $EXIT_CODE
1 change: 1 addition & 0 deletions .sourcekit-lsp/config.json
1 change: 1 addition & 0 deletions Benchmarks/.gitignore
1 change: 1 addition & 0 deletions Benchmarks/.sourcekit-lsp
1 change: 1 addition & 0 deletions Benchmarks/.swift-format
40 changes: 40 additions & 0 deletions Benchmarks/.vscode/launch.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"configurations": [
{
"type": "swift-lldb",
"request": "launch",
"args": [],
"cwd": "${workspaceFolder:Benchmarks}",
"name": "Debug Serializer",
"program": "${workspaceFolder:Benchmarks}/.build/debug/Serializer",
"preLaunchTask": "swift: Build Debug Serializer"
},
{
"type": "swift-lldb",
"request": "launch",
"args": [],
"cwd": "${workspaceFolder:Benchmarks}",
"name": "Release Serializer",
"program": "${workspaceFolder:Benchmarks}/.build/release/Serializer",
"preLaunchTask": "swift: Build Release Serializer"
},
{
"type": "swift-lldb",
"request": "launch",
"args": [],
"cwd": "${workspaceFolder:Benchmarks}",
"name": "Debug Parser",
"program": "${workspaceFolder:Benchmarks}/.build/debug/Parser",
"preLaunchTask": "swift: Build Debug Parser"
},
{
"type": "swift-lldb",
"request": "launch",
"args": [],
"cwd": "${workspaceFolder:Benchmarks}",
"name": "Release Parser",
"program": "${workspaceFolder:Benchmarks}/.build/release/Parser",
"preLaunchTask": "swift: Build Release Parser"
}
]
}
38 changes: 38 additions & 0 deletions Benchmarks/Package.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// swift-tools-version:6.0

import PackageDescription

let package = Package(
name: "Benchmarks",
platforms: [
.macOS(.v13)
],
dependencies: [
.package(path: "../"),
.package(url: "https://github.com/ordo-one/package-benchmark.git", from: "1.27.0"),
],
targets: [
.executableTarget(
name: "Serializer",
dependencies: [
.product(name: "MultipartKit", package: "multipart-kit"),
.product(name: "Benchmark", package: "package-benchmark"),
],
path: "Serializer",
plugins: [
.plugin(name: "BenchmarkPlugin", package: "package-benchmark")
]
),
.executableTarget(
name: "Parser",
dependencies: [
.product(name: "MultipartKit", package: "multipart-kit"),
.product(name: "Benchmark", package: "package-benchmark"),
],
path: "Parser",
plugins: [
.plugin(name: "BenchmarkPlugin", package: "package-benchmark")
]
),
]
)
37 changes: 37 additions & 0 deletions Benchmarks/Parser/AsyncSyncSequence.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
extension Sequence {
var async: AsyncSyncSequence<Self> {
AsyncSyncSequence(self)
}
}

/// An asynchronous sequence composed from a synchronous sequence, that releases the reference to
/// the base sequence when an iterator is created. So you can only iterate once.
///
/// Not safe. Only for testing purposes.
/// Use `swift-algorithms`'s `AsyncSyncSequence`` instead if you're looking for something like this.
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not release the reference when an iterator is created, so it triggers CoW.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean i know why it doesn't, and it shouldn't, but that's not what we need. We want the benchmarks to be self-contained and only have minimum required overhead. If someone is using some kind of sequence with Multipart-Kit that is inefficient, then that's not our fault.

final class AsyncSyncSequence<Base: Sequence>: AsyncSequence {
typealias Element = Base.Element

struct Iterator: AsyncIteratorProtocol {
var iterator: Base.Iterator?

init(_ iterator: Base.Iterator) {
self.iterator = iterator
}

mutating func next() async -> Base.Element? {
iterator?.next()
}
}

private var base: Base?

init(_ base: Base) {
self.base = base
}

func makeAsyncIterator() -> Iterator {
defer { self.base = nil } // release the reference so no CoW is triggered
return Iterator(base.unsafelyUnwrapped.makeIterator())
}
}
Loading
Loading