From 230aa89b30b2d5cf311af102b79bc0f833f055d5 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 10:41:00 -0500 Subject: [PATCH 01/24] ci: fix file juggling in bench job --- .github/workflows/bench.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 1773ae4..65d18b1 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -40,9 +40,11 @@ jobs: # restores the same file path and we need to keep it around for # benchstat comparison before saving new results with the same name. run: | + if [ -f bench-main.txt ]; then + mv -f bench-main-curr.txt bench-main-prev.txt + fi + make bench | tee bench-main-curr.txt - mv -f bench-main.txt bench-main-prev.txt - cp bench-main-curr.txt bench-main.txt CURR_VERSION="${GITHUB_SHA::7}" CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" @@ -51,6 +53,7 @@ jobs: # record commit for which the benchmarks were run echo -n "$CURR_VERSION" > bench-version.txt + # report results echo "### benchmarks: $CURR_LINK" >>$GITHUB_STEP_SUMMARY echo '```' >>$GITHUB_STEP_SUMMARY cat bench-main-curr.txt >>$GITHUB_STEP_SUMMARY From 20933c6a606cad5d4872e8fca494101d99d633fd Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 10:41:41 -0500 Subject: [PATCH 02/24] TMP enable job on branch --- .github/workflows/bench.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 65d18b1..0c0b947 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -19,7 +19,7 @@ env: jobs: baseline: runs-on: ubuntu-latest - if: ${{ github.ref == 'refs/heads/main' }} + # if: ${{ github.ref == 'refs/heads/main' }} steps: - uses: actions/setup-go@v5 with: From 59c079f63bb10584bc4363258534c0fe136e4d45 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 21:49:00 -0500 Subject: [PATCH 03/24] fix conditional --- .github/workflows/bench.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 0c0b947..67113c3 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -60,7 +60,7 @@ jobs: echo '```' >>$GITHUB_STEP_SUMMARY - name: run prev benchmarks if necessary - if: ${{ steps.baseline-restore.outputs.cache-hit != '' }} + if: ${{ steps.baseline-restore.outputs.cache-hit == '' }} run: | # Determine the base SHA depending on the event type if [ "${{ github.event_name }}" = "pull_request" ]; then From 7034a3e6acb399800d3fb588059a9cc122f81849 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 21:53:20 -0500 Subject: [PATCH 04/24] fix --- .github/workflows/bench.yaml | 2 +- websocket_benchmark_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 67113c3..157e7c7 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -40,7 +40,7 @@ jobs: # restores the same file path and we need to keep it around for # benchstat comparison before saving new results with the same name. run: | - if [ -f bench-main.txt ]; then + if [ -f bench-main-curr.txt ]; then mv -f bench-main-curr.txt bench-main-prev.txt fi diff --git a/websocket_benchmark_test.go b/websocket_benchmark_test.go index e408309..f911eb5 100644 --- a/websocket_benchmark_test.go +++ b/websocket_benchmark_test.go @@ -28,9 +28,9 @@ func makeFrame(opcode websocket.Opcode, fin bool, payloadLen int) *websocket.Fra func BenchmarkReadFrame(b *testing.B) { frameSizes := []int{ - 256, + // 256, 1024, - 256 * 1024, + // 256 * 1024, 1024 * 1024, // largest cases from the autobahn test suite 8 * 1024 * 1024, From 1f014b254b807835d7a0bab963a8887f91e665ec Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 22:03:24 -0500 Subject: [PATCH 05/24] debug --- .github/workflows/bench.yaml | 6 +++++- Makefile | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 157e7c7..0564648 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -36,6 +36,7 @@ jobs: key: ${{ runner.os }}-bench-main - name: run current benchmarks + id: crrent-benchmarks # We have to juggle file names here because the cache action saves and # restores the same file path and we need to keep it around for # benchstat comparison before saving new results with the same name. @@ -59,6 +60,9 @@ jobs: cat bench-main-curr.txt >>$GITHUB_STEP_SUMMARY echo '```' >>$GITHUB_STEP_SUMMARY + echo "XXX summary:" + cat $GITHUB_STEP_SUMMARY + - name: run prev benchmarks if necessary if: ${{ steps.baseline-restore.outputs.cache-hit == '' }} run: | @@ -94,7 +98,7 @@ jobs: echo '```' >>$GITHUB_STEP_SUMMARY - name: save new baseline results - id: baseline-save + if: steps.current-benchmarks.outcome == 'success' uses: actions/cache/save@v4 with: path: | diff --git a/Makefile b/Makefile index 0090a23..6359506 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ COVERAGE_PATH ?= coverage.out COVERAGE_ARGS ?= -covermode=atomic -coverprofile=$(COVERAGE_PATH) TEST_ARGS ?= -race -timeout 60s -count=1 -BENCH_COUNT ?= 5 +BENCH_COUNT ?= 10 BENCH_ARGS ?= -bench=. -benchmem -count=$(BENCH_COUNT) # 3rd party tools @@ -56,7 +56,7 @@ testautobahn: bench: go test $(BENCH_ARGS) .PHONY: bench - + lint: test -z "$$($(CMD_GOFUMPT) -d -e .)" || (echo "Error: gofmt failed"; $(CMD_GOFUMPT) -d -e . ; exit 1) go vet ./... From a8ddffdb66801bfa789bc03c2c6342b69ce3a257 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 22:10:06 -0500 Subject: [PATCH 06/24] more fixes --- .github/workflows/bench.yaml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 0564648..1b7e7fe 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -80,9 +80,8 @@ jobs: # TODO: cache benchstat - name: compare results with benchstat - id: benchstat run: | - go run golang.org/x/perf/cmd/benchstat@latest bench-main-prev.txt bench-main-curr.txt | tee -a $GITHUB_OUTPUT bench-stats.txt + go run golang.org/x/perf/cmd/benchstat@latest bench-main-prev.txt bench-main-curr.txt | tee -a bench-stats.txt CURR_VERSION="${GITHUB_SHA::7}" CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" @@ -98,7 +97,7 @@ jobs: echo '```' >>$GITHUB_STEP_SUMMARY - name: save new baseline results - if: steps.current-benchmarks.outcome == 'success' + if: ${{ steps.current-benchmarks.outcome == 'success' }} uses: actions/cache/save@v4 with: path: | From cde467a4c55e4af89a6a96b6d3cb3b8d66b21578 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 22:16:16 -0500 Subject: [PATCH 07/24] come on --- .github/workflows/bench.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 1b7e7fe..81d1251 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -36,7 +36,7 @@ jobs: key: ${{ runner.os }}-bench-main - name: run current benchmarks - id: crrent-benchmarks + id: current-benchmarks # We have to juggle file names here because the cache action saves and # restores the same file path and we need to keep it around for # benchstat comparison before saving new results with the same name. From 33d158d6b124790b033fd0ca9b2aeecc745c09fe Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Fri, 3 Jan 2025 22:21:45 -0500 Subject: [PATCH 08/24] compare urls --- .github/workflows/bench.yaml | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 81d1251..8179b23 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -84,17 +84,14 @@ jobs: go run golang.org/x/perf/cmd/benchstat@latest bench-main-prev.txt bench-main-curr.txt | tee -a bench-stats.txt CURR_VERSION="${GITHUB_SHA::7}" - CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" - CURR_LINK="[$CURR_VERSION]($CURR_URL)" - PREV_VERSION="$(cat bench-version.txt 2>/dev/null)" - PREV_URL="https://github.com/mccutchen/websocket/commit/$PREV_VERSION" - PREV_LINK="[$PREV_VERSION]($PREV_URL)" + COMPARE_URL="https://github.com/mccutchen/websocket/compare/$PREV_VERSION...$CURR_VERSION" + COMPARE_LINK="[$PREV_VERSION...$CURR_VERSION]($COMPARE_URL)" - echo "### benchstats: $PREV_LINK (old) vs $CURR_LINK (new)" >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY - cat bench-stats.txt >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY + echo "### benchstats: $COMPARE_LINK" >>$GITHUB_STEP_SUMMARY + echo '```' >>$GITHUB_STEP_SUMMARY + cat bench-stats.txt >>$GITHUB_STEP_SUMMARY + echo '```' >>$GITHUB_STEP_SUMMARY - name: save new baseline results if: ${{ steps.current-benchmarks.outcome == 'success' }} From 703ff81bdb59e97ddc66ea60e02e3a82d507b8e4 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 08:29:10 -0500 Subject: [PATCH 09/24] different cache key? --- .github/workflows/bench.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 8179b23..889a360 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -33,7 +33,7 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-main + key: ${{ runner.os }}-bench-results - name: run current benchmarks id: current-benchmarks @@ -99,4 +99,4 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-main + key: ${{ runner.os }}-bench-results From 4e1c7abd0c6799d98f3a055773ee754801819de7 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 08:41:15 -0500 Subject: [PATCH 10/24] fix version handling --- .github/workflows/bench.yaml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 889a360..d2f6240 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -42,7 +42,10 @@ jobs: # benchstat comparison before saving new results with the same name. run: | if [ -f bench-main-curr.txt ]; then - mv -f bench-main-curr.txt bench-main-prev.txt + mv bench-main-curr.txt bench-main-prev.txt + fi + if [ -f bench-version-curr.txt ]; then + mv bench-version-curr.txt bench-version-prev.txt fi make bench | tee bench-main-curr.txt @@ -52,7 +55,7 @@ jobs: CURR_LINK="[$CURR_VERSION]($CURR_URL)" # record commit for which the benchmarks were run - echo -n "$CURR_VERSION" > bench-version.txt + echo -n "$CURR_VERSION" > bench-version-curr.txt # report results echo "### benchmarks: $CURR_LINK" >>$GITHUB_STEP_SUMMARY @@ -72,6 +75,7 @@ jobs: else BASE_SHA=$(git rev-parse HEAD~1) fi + echo $BASE_SHA > bench-version-prev.txt git fetch origin main $BASE_SHA git reset --hard $BASE_SHA @@ -84,7 +88,7 @@ jobs: go run golang.org/x/perf/cmd/benchstat@latest bench-main-prev.txt bench-main-curr.txt | tee -a bench-stats.txt CURR_VERSION="${GITHUB_SHA::7}" - PREV_VERSION="$(cat bench-version.txt 2>/dev/null)" + PREV_VERSION="$(cat bench-version-prev.txt 2>/dev/null)" COMPARE_URL="https://github.com/mccutchen/websocket/compare/$PREV_VERSION...$CURR_VERSION" COMPARE_LINK="[$PREV_VERSION...$CURR_VERSION]($COMPARE_URL)" From 33d3ac7d318400473e4a334d464c7658a9773c62 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 08:41:22 -0500 Subject: [PATCH 11/24] pr benchmark job? --- .github/workflows/bench.yaml | 59 ++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index d2f6240..dffcc08 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -104,3 +104,62 @@ jobs: path: | bench-*.txt key: ${{ runner.os }}-bench-results + + pr: + runs-on: ubuntu-latest + if: ${{ github.ref != 'refs/heads/main' }} + steps: + - uses: actions/setup-go@v5 + with: + go-version: "stable" + + - uses: actions/checkout@v4 + + - name: restore previous baseline results + id: baseline-restore + uses: actions/cache/restore@v4 + with: + path: | + bench-*.txt + key: ${{ runner.os }}-bench-results + + - name: run current benchmarks + id: current-benchmarks + # We have to juggle file names here because the cache action saves and + # restores the same file path and we need to keep it around for + # benchstat comparison before saving new results with the same name. + run: | + CURR_VERSION="${GITHUB_SHA::7}" + OUT_PATH="bench-commit-${CURR_VERSION}.txt" + + make bench | tee $OUT_PATH + + CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" + CURR_LINK="[$CURR_VERSION]($CURR_URL)" + + # report results + echo "### benchmarks @ $CURR_LINK" >>pr_comment + echo '```' >>pr_comment + cat $OUT_PATH >>pr_comment + echo '```' >>pr_comment + + # TODO: cache benchstat + - name: compare results with benchstat + if: ${{ steps.baseline-restore.outputs.cache-hit != '' }} + run: | + go run golang.org/x/perf/cmd/benchstat@latest bench-main-curr.txt bench-commit-*.txt | tee -a bench-stats.txt + + CURR_VERSION="${GITHUB_SHA::7}" + PREV_VERSION="$(cat bench-version-prev.txt 2>/dev/null)" + COMPARE_URL="https://github.com/mccutchen/websocket/compare/$PREV_VERSION...$CURR_VERSION" + COMPARE_LINK="[$PREV_VERSION...$CURR_VERSION]($COMPARE_URL)" + + echo "### benchstats: $COMPARE_LINK" >>pr_comment + echo '```' >>pr_comment + cat bench-stats.txt >>pr_comment + echo '```' >>pr_comment + + - name: post benchmark results + uses: marocchino/sticky-pull-request-comment@v2 + with: + path: pr_comment From 5bdf176280ad976c66018e1df4fdd991a38da61a Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 08:47:42 -0500 Subject: [PATCH 12/24] debug --- .github/workflows/bench.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index dffcc08..cd93d2d 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -85,13 +85,14 @@ jobs: # TODO: cache benchstat - name: compare results with benchstat run: | - go run golang.org/x/perf/cmd/benchstat@latest bench-main-prev.txt bench-main-curr.txt | tee -a bench-stats.txt - CURR_VERSION="${GITHUB_SHA::7}" PREV_VERSION="$(cat bench-version-prev.txt 2>/dev/null)" COMPARE_URL="https://github.com/mccutchen/websocket/compare/$PREV_VERSION...$CURR_VERSION" COMPARE_LINK="[$PREV_VERSION...$CURR_VERSION]($COMPARE_URL)" + echo "Comparing $PREV_VERSION -> $CURR_VERSION ..." + go run golang.org/x/perf/cmd/benchstat@latest bench-main-prev.txt bench-main-curr.txt | tee -a bench-stats.txt + echo "### benchstats: $COMPARE_LINK" >>$GITHUB_STEP_SUMMARY echo '```' >>$GITHUB_STEP_SUMMARY cat bench-stats.txt >>$GITHUB_STEP_SUMMARY @@ -107,7 +108,7 @@ jobs: pr: runs-on: ubuntu-latest - if: ${{ github.ref != 'refs/heads/main' }} + if: ${{ github.ref != 'refs/heads/main XXX FIXME' }} steps: - uses: actions/setup-go@v5 with: From 6ff8b14c538ceed4205483a458182b0adb58bee7 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 08:55:07 -0500 Subject: [PATCH 13/24] better cache key? --- .github/workflows/bench.yaml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index cd93d2d..f736d1c 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -33,7 +33,7 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml'}} - name: run current benchmarks id: current-benchmarks @@ -104,11 +104,12 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml'}} pr: runs-on: ubuntu-latest - if: ${{ github.ref != 'refs/heads/main XXX FIXME' }} + # if: ${{ github.ref != 'refs/heads/main' }} + if: ${{ github.ref == 'XXX' }} steps: - uses: actions/setup-go@v5 with: @@ -122,7 +123,7 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml'}} - name: run current benchmarks id: current-benchmarks From d355f604039ca530d0d35530540bf797280ebaf5 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 08:57:45 -0500 Subject: [PATCH 14/24] revisit readmessage bench strategy --- websocket_benchmark_test.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/websocket_benchmark_test.go b/websocket_benchmark_test.go index f911eb5..9b41cbf 100644 --- a/websocket_benchmark_test.go +++ b/websocket_benchmark_test.go @@ -64,13 +64,16 @@ func BenchmarkReadMessage(b *testing.B) { msgSize int frameCount int }{ - {16 * 1024, 4}, - {16 * 1024, 16}, - {1024 * 1024, 4}, - // worst case sizes from autobahn test suite + {1024 * 1024, 1}, {8 * 1024 * 1024, 1}, - {8 * 1024 * 1024, 8}, {16 * 1024 * 1024, 1}, + + {1024 * 1024, 4}, + {8 * 1024 * 1024, 4}, + {16 * 1024 * 1024, 4}, + + {1024 * 1024, 15}, + {8 * 1024 * 1024, 16}, {16 * 1024 * 1024, 16}, } From e3ebbff6145085a8724f442ab5df2d141cb4bf80 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 09:01:37 -0500 Subject: [PATCH 15/24] syntax --- .github/workflows/bench.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index f736d1c..5d00a93 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -33,7 +33,7 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml'}} + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml') }} - name: run current benchmarks id: current-benchmarks @@ -104,7 +104,7 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml'}} + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml') }} pr: runs-on: ubuntu-latest @@ -123,7 +123,7 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml'}} + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml') }} - name: run current benchmarks id: current-benchmarks From 49c164fe45dc08a74b8538e7a58769bc848c6433 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 09:05:31 -0500 Subject: [PATCH 16/24] ugh --- websocket_benchmark_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/websocket_benchmark_test.go b/websocket_benchmark_test.go index 9b41cbf..514a5b3 100644 --- a/websocket_benchmark_test.go +++ b/websocket_benchmark_test.go @@ -72,7 +72,7 @@ func BenchmarkReadMessage(b *testing.B) { {8 * 1024 * 1024, 4}, {16 * 1024 * 1024, 4}, - {1024 * 1024, 15}, + {1024 * 1024, 16}, {8 * 1024 * 1024, 16}, {16 * 1024 * 1024, 16}, } From 5e015b5dc1b044539c27b93c132bbfb7d520a61b Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 17:45:42 -0500 Subject: [PATCH 17/24] enable real jobs? --- .github/workflows/bench.yaml | 95 ++++++++++++++---------------------- websocket_benchmark_test.go | 3 ++ 2 files changed, 40 insertions(+), 58 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 5d00a93..f47ef43 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -33,23 +33,13 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml') }} + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go') }} - - name: run current benchmarks - id: current-benchmarks + - name: run benchmarks # We have to juggle file names here because the cache action saves and # restores the same file path and we need to keep it around for # benchstat comparison before saving new results with the same name. run: | - if [ -f bench-main-curr.txt ]; then - mv bench-main-curr.txt bench-main-prev.txt - fi - if [ -f bench-version-curr.txt ]; then - mv bench-version-curr.txt bench-version-prev.txt - fi - - make bench | tee bench-main-curr.txt - CURR_VERSION="${GITHUB_SHA::7}" CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" CURR_LINK="[$CURR_VERSION]($CURR_URL)" @@ -57,6 +47,8 @@ jobs: # record commit for which the benchmarks were run echo -n "$CURR_VERSION" > bench-version-curr.txt + make bench | tee bench-main-curr.txt + # report results echo "### benchmarks: $CURR_LINK" >>$GITHUB_STEP_SUMMARY echo '```' >>$GITHUB_STEP_SUMMARY @@ -66,50 +58,16 @@ jobs: echo "XXX summary:" cat $GITHUB_STEP_SUMMARY - - name: run prev benchmarks if necessary - if: ${{ steps.baseline-restore.outputs.cache-hit == '' }} - run: | - # Determine the base SHA depending on the event type - if [ "${{ github.event_name }}" = "pull_request" ]; then - BASE_SHA=${{ github.event.pull_request.base.sha }} - else - BASE_SHA=$(git rev-parse HEAD~1) - fi - echo $BASE_SHA > bench-version-prev.txt - - git fetch origin main $BASE_SHA - git reset --hard $BASE_SHA - make bench | tee bench-main-prev.txt - git reset --hard $GITHUB_SHA - - # TODO: cache benchstat - - name: compare results with benchstat - run: | - CURR_VERSION="${GITHUB_SHA::7}" - PREV_VERSION="$(cat bench-version-prev.txt 2>/dev/null)" - COMPARE_URL="https://github.com/mccutchen/websocket/compare/$PREV_VERSION...$CURR_VERSION" - COMPARE_LINK="[$PREV_VERSION...$CURR_VERSION]($COMPARE_URL)" - - echo "Comparing $PREV_VERSION -> $CURR_VERSION ..." - go run golang.org/x/perf/cmd/benchstat@latest bench-main-prev.txt bench-main-curr.txt | tee -a bench-stats.txt - - echo "### benchstats: $COMPARE_LINK" >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY - cat bench-stats.txt >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY - - name: save new baseline results - if: ${{ steps.current-benchmarks.outcome == 'success' }} uses: actions/cache/save@v4 with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml') }} + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go') }} pr: runs-on: ubuntu-latest - # if: ${{ github.ref != 'refs/heads/main' }} - if: ${{ github.ref == 'XXX' }} + if: ${{ github.ref != 'refs/heads/main' }} steps: - uses: actions/setup-go@v5 with: @@ -123,27 +81,48 @@ jobs: with: path: | bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go', '.github/workflows/bench.yaml') }} + key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go') }} - - name: run current benchmarks - id: current-benchmarks + - name: benchmark current commit # We have to juggle file names here because the cache action saves and # restores the same file path and we need to keep it around for # benchstat comparison before saving new results with the same name. run: | - CURR_VERSION="${GITHUB_SHA::7}" - OUT_PATH="bench-commit-${CURR_VERSION}.txt" + if [ -f bench-main-curr.txt ]; then + mv bench-main-curr.txt bench-main-prev.txt + fi + if [ -f bench-version-curr.txt ]; then + mv bench-version-curr.txt bench-version-prev.txt + fi - make bench | tee $OUT_PATH + make bench | tee bench-main-curr.txt + CURR_VERSION="${GITHUB_SHA::7}" CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" CURR_LINK="[$CURR_VERSION]($CURR_URL)" + # record commit for which the benchmarks were run + echo -n "$CURR_VERSION" > bench-version-curr.txt + # report results - echo "### benchmarks @ $CURR_LINK" >>pr_comment - echo '```' >>pr_comment - cat $OUT_PATH >>pr_comment - echo '```' >>pr_comment + echo "### benchmarks: $CURR_LINK" >>$GITHUB_STEP_SUMMARY + echo '```' >>$GITHUB_STEP_SUMMARY + cat bench-main-curr.txt >>$GITHUB_STEP_SUMMARY + echo '```' >>$GITHUB_STEP_SUMMARY + + echo "XXX summary:" + cat $GITHUB_STEP_SUMMARY + + - name: benchmark prev commit if necessary + if: ${{ steps.baseline-restore.outputs.cache-hit == '' }} + run: | + BASE_SHA=${{ github.event.pull_request.base.sha }} + echo $BASE_SHA > bench-version-prev.txt + + git fetch origin main $BASE_SHA + git reset --hard $BASE_SHA + make bench | tee bench-main-prev.txt + git reset --hard $GITHUB_SHA # TODO: cache benchstat - name: compare results with benchstat diff --git a/websocket_benchmark_test.go b/websocket_benchmark_test.go index 514a5b3..5b31e21 100644 --- a/websocket_benchmark_test.go +++ b/websocket_benchmark_test.go @@ -64,14 +64,17 @@ func BenchmarkReadMessage(b *testing.B) { msgSize int frameCount int }{ + // 1 frame per message {1024 * 1024, 1}, {8 * 1024 * 1024, 1}, {16 * 1024 * 1024, 1}, + // 4 frames per message {1024 * 1024, 4}, {8 * 1024 * 1024, 4}, {16 * 1024 * 1024, 4}, + // 16 frames per message {1024 * 1024, 16}, {8 * 1024 * 1024, 16}, {16 * 1024 * 1024, 16}, From a622bfe25aefe1b88ecc6cb523e3e4a6c0805b06 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 17:53:49 -0500 Subject: [PATCH 18/24] test --- .github/workflows/bench.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index f47ef43..ee49302 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -140,6 +140,11 @@ jobs: cat bench-stats.txt >>pr_comment echo '```' >>pr_comment + echo "XXX FINAL PR COMMENT:" + echo "================================================================================" + cat pr_comment + echo "================================================================================" + - name: post benchmark results uses: marocchino/sticky-pull-request-comment@v2 with: From f185504ee48c80607c9c4453617172f92e23cf21 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 17:54:54 -0500 Subject: [PATCH 19/24] fix --- .github/workflows/bench.yaml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index ee49302..8f72de6 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -105,13 +105,11 @@ jobs: echo -n "$CURR_VERSION" > bench-version-curr.txt # report results - echo "### benchmarks: $CURR_LINK" >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY - cat bench-main-curr.txt >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY - - echo "XXX summary:" - cat $GITHUB_STEP_SUMMARY + echo "### benchmarks: $CURR_LINK" >>pr_comment + echo '```' >>pr_comment + cat bench-main-curr.txt >>pr_comment + echo '```' >>pr_comment + cat pr_comment >> $GITHUB_STEP_SUMMARY - name: benchmark prev commit if necessary if: ${{ steps.baseline-restore.outputs.cache-hit == '' }} From 6fe249625a6059e63777c3aa95af5c02e39ddda9 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 17:57:53 -0500 Subject: [PATCH 20/24] no longer need this comment --- .github/workflows/bench.yaml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 8f72de6..c7ac9db 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -36,9 +36,6 @@ jobs: key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go') }} - name: run benchmarks - # We have to juggle file names here because the cache action saves and - # restores the same file path and we need to keep it around for - # benchstat comparison before saving new results with the same name. run: | CURR_VERSION="${GITHUB_SHA::7}" CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" From 0b59dcbcd5dc9e59c46a9f714c95d366d382e9ec Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 18:05:06 -0500 Subject: [PATCH 21/24] permissions --- .github/workflows/bench.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index c7ac9db..d70d9d5 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -65,6 +65,8 @@ jobs: pr: runs-on: ubuntu-latest if: ${{ github.ref != 'refs/heads/main' }} + permissions: + pull-requests: write steps: - uses: actions/setup-go@v5 with: From 8cac32a762e6c542e88777035a1a2cbf4d30c89d Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sat, 4 Jan 2025 18:06:09 -0500 Subject: [PATCH 22/24] document perms --- .github/workflows/bench.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index d70d9d5..ef0e975 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -65,8 +65,12 @@ jobs: pr: runs-on: ubuntu-latest if: ${{ github.ref != 'refs/heads/main' }} + + # allow commenting on PR: + # https://github.com/marocchino/sticky-pull-request-comment/blob/main/README.md#error-resource-not-accessible-by-integration permissions: pull-requests: write + steps: - uses: actions/setup-go@v5 with: From f853263c54f6a10f1d6e50e40be0eebd0c61d226 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sun, 5 Jan 2025 08:47:25 -0500 Subject: [PATCH 23/24] simplify baseline for merge --- .github/workflows/bench.yaml | 135 +++++------------------------------ 1 file changed, 18 insertions(+), 117 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index ef0e975..49c611b 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -19,7 +19,7 @@ env: jobs: baseline: runs-on: ubuntu-latest - # if: ${{ github.ref == 'refs/heads/main' }} + if: ${{ github.ref == 'refs/heads/main' }} steps: - uses: actions/setup-go@v5 with: @@ -27,126 +27,27 @@ jobs: - uses: actions/checkout@v4 - - name: restore previous baseline results - id: baseline-restore - uses: actions/cache/restore@v4 + - name: cache previous baseline results + uses: actions/cache@v4 with: path: | bench-*.txt key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go') }} + restore-keys: | + ${{ runner.os }}-bench-results- - name: run benchmarks run: | - CURR_VERSION="${GITHUB_SHA::7}" - CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" - CURR_LINK="[$CURR_VERSION]($CURR_URL)" - - # record commit for which the benchmarks were run - echo -n "$CURR_VERSION" > bench-version-curr.txt - - make bench | tee bench-main-curr.txt - - # report results - echo "### benchmarks: $CURR_LINK" >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY - cat bench-main-curr.txt >>$GITHUB_STEP_SUMMARY - echo '```' >>$GITHUB_STEP_SUMMARY - - echo "XXX summary:" - cat $GITHUB_STEP_SUMMARY - - - name: save new baseline results - uses: actions/cache/save@v4 - with: - path: | - bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go') }} - - pr: - runs-on: ubuntu-latest - if: ${{ github.ref != 'refs/heads/main' }} - - # allow commenting on PR: - # https://github.com/marocchino/sticky-pull-request-comment/blob/main/README.md#error-resource-not-accessible-by-integration - permissions: - pull-requests: write - - steps: - - uses: actions/setup-go@v5 - with: - go-version: "stable" - - - uses: actions/checkout@v4 - - - name: restore previous baseline results - id: baseline-restore - uses: actions/cache/restore@v4 - with: - path: | - bench-*.txt - key: ${{ runner.os }}-bench-results-${{ hashFiles('websocket_benchmark_test.go') }} - - - name: benchmark current commit - # We have to juggle file names here because the cache action saves and - # restores the same file path and we need to keep it around for - # benchstat comparison before saving new results with the same name. - run: | - if [ -f bench-main-curr.txt ]; then - mv bench-main-curr.txt bench-main-prev.txt - fi - if [ -f bench-version-curr.txt ]; then - mv bench-version-curr.txt bench-version-prev.txt - fi - - make bench | tee bench-main-curr.txt - - CURR_VERSION="${GITHUB_SHA::7}" - CURR_URL="https://github.com/mccutchen/websocket/commit/$CURR_VERSION" - CURR_LINK="[$CURR_VERSION]($CURR_URL)" - - # record commit for which the benchmarks were run - echo -n "$CURR_VERSION" > bench-version-curr.txt - - # report results - echo "### benchmarks: $CURR_LINK" >>pr_comment - echo '```' >>pr_comment - cat bench-main-curr.txt >>pr_comment - echo '```' >>pr_comment - cat pr_comment >> $GITHUB_STEP_SUMMARY - - - name: benchmark prev commit if necessary - if: ${{ steps.baseline-restore.outputs.cache-hit == '' }} - run: | - BASE_SHA=${{ github.event.pull_request.base.sha }} - echo $BASE_SHA > bench-version-prev.txt - - git fetch origin main $BASE_SHA - git reset --hard $BASE_SHA - make bench | tee bench-main-prev.txt - git reset --hard $GITHUB_SHA - - # TODO: cache benchstat - - name: compare results with benchstat - if: ${{ steps.baseline-restore.outputs.cache-hit != '' }} - run: | - go run golang.org/x/perf/cmd/benchstat@latest bench-main-curr.txt bench-commit-*.txt | tee -a bench-stats.txt - - CURR_VERSION="${GITHUB_SHA::7}" - PREV_VERSION="$(cat bench-version-prev.txt 2>/dev/null)" - COMPARE_URL="https://github.com/mccutchen/websocket/compare/$PREV_VERSION...$CURR_VERSION" - COMPARE_LINK="[$PREV_VERSION...$CURR_VERSION]($COMPARE_URL)" - - echo "### benchstats: $COMPARE_LINK" >>pr_comment - echo '```' >>pr_comment - cat bench-stats.txt >>pr_comment - echo '```' >>pr_comment - - echo "XXX FINAL PR COMMENT:" - echo "================================================================================" - cat pr_comment - echo "================================================================================" - - - name: post benchmark results - uses: marocchino/sticky-pull-request-comment@v2 - with: - path: pr_comment + COMMIT="${GITHUB_SHA::7}" + COMMIT_URL="https://github.com/mccutchen/websocket/commit/$COMMIT" + COMMIT_LINK="[$COMMIT]($COMMIT_URL)" + + echo -n "$COMMIT" > bench-commit-main.txt + make bench | tee bench-results-main.txt + + cat <>$GITHUB_STEP_SUMMARY + ### benchmark results @ $COMMIT_LINK + \`\`\` + $(cat bench-results-main.txt) + \`\`\` + EOF From 67e85fd1dc0ea296550084c511cb4f562702f316 Mon Sep 17 00:00:00 2001 From: Will McCutchen Date: Sun, 5 Jan 2025 08:50:02 -0500 Subject: [PATCH 24/24] cleanup --- .github/workflows/bench.yaml | 3 --- websocket_benchmark_test.go | 2 -- 2 files changed, 5 deletions(-) diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml index 49c611b..f7b9a83 100644 --- a/.github/workflows/bench.yaml +++ b/.github/workflows/bench.yaml @@ -13,9 +13,6 @@ concurrency: group: "${{ github.workflow }}-${{ github.ref }}" cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} -env: - BENCH_RUNS: 10 - jobs: baseline: runs-on: ubuntu-latest diff --git a/websocket_benchmark_test.go b/websocket_benchmark_test.go index 5b31e21..2f5ee22 100644 --- a/websocket_benchmark_test.go +++ b/websocket_benchmark_test.go @@ -28,9 +28,7 @@ func makeFrame(opcode websocket.Opcode, fin bool, payloadLen int) *websocket.Fra func BenchmarkReadFrame(b *testing.B) { frameSizes := []int{ - // 256, 1024, - // 256 * 1024, 1024 * 1024, // largest cases from the autobahn test suite 8 * 1024 * 1024,