Run benchmarks nightly and cover the LLAPI (search) path - #1629
Conversation
|
|
The benchmark suite only ran on demand, via the `run-benchmark` PR label, so
nothing measured performance ahead of a release. Add a `perf-nightly` job to
the nightly event that calls the existing benchmark runner, and include it in
the test summary so regressions reach the same Slack report as the rest.
None of the existing 44 specs touched the shared C API (the LLAPI): they drive
JSON.SET/GET/ARRAPPEND/NUMINCRBY/NUMMULTBY only. The surface RediSearch
actually consumes - getAt/getArray - was therefore unmeasured, and it is not
reachable over the protocol at all, being an in-process C ABI. The LLAPI test
consumer module already exposes it as LLAPI.* commands, so load that module
alongside the module under test and benchmark through it:
- llapi_getat_scan_packed_f32_{320,1280}: reads a packed f32 array element by
element via getAt(), the pattern a vector read uses. The pair exists so the
per-element cost can be compared across a 4x size jump - a per-element cost
that grows with array length is the signal, which no absolute rps number
gives you.
- llapi_getat_last_packed_f32_1280: one indexed read at the last index.
- llapi_getarray_packed_f32_1280: the bulk buffer path, as a baseline.
LLAPI.GETAT_SCAN is new in the test module; the vectors are built by an
init_commands EVAL, whose values are chosen to be f32-exact but outside f16
range so the array packs as f32 rather than a narrower type.
The .so build recipe moves out of tests.sh into llapi_test_module/build.sh so
the flow tests and the benchmark flow share one builder and cannot drift.
80ba510 to
7e487d9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1629 +/- ##
=======================================
Coverage 85.97% 85.97%
=======================================
Files 15 15
Lines 5305 5305
=======================================
Hits 4561 4561
Misses 744 744 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7e487d9. Configure here.
| japi->freeJson(buf); | ||
| japi->freeIter(it); | ||
| RedisModule_ReplyWithLongLong(ctx, read); | ||
| return REDISMODULE_OK; |
There was a problem hiding this comment.
Partial scan reported as success
Medium Severity
LLAPI.GETAT_SCAN breaks out of the getAt loop on failure but still replies with a successful integer count. Unlike LLAPI.GETAT, which returns an error when getAt fails, a mid-scan failure looks like a normal reply to redis-benchmark, so nightly LLAPI specs can keep reporting healthy ops/sec while indexed reads are broken.
Reviewed by Cursor Bugbot for commit 7e487d9. Configure here.
| redis-ref: ${{ needs.prepare-values.outputs.redis-ref }} | ||
| send-slack-message: ${{ github.event_name == 'schedule' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main') || (github.event_name == 'workflow_dispatch' && inputs.send-slack-message == true) }} | ||
| job-failures: ${{ needs.build-linux-x64.result == 'failure' && 'build-linux-x64 ' || '' }}${{ needs.build-linux-arm64.result == 'failure' && 'build-linux-arm64 ' || '' }}${{ needs.macos.result == 'failure' && 'macos ' || '' }}${{ needs.linux-valgrind.result == 'failure' && 'linux-valgrind ' || '' }}${{ needs.linux-sanitizer.result == 'failure' && 'linux-sanitizer ' || '' }}${{ needs.random-traffic.result == 'failure' && 'random-traffic' || '' }} | ||
| job-failures: ${{ needs.build-linux-x64.result == 'failure' && 'build-linux-x64 ' || '' }}${{ needs.build-linux-arm64.result == 'failure' && 'build-linux-arm64 ' || '' }}${{ needs.macos.result == 'failure' && 'macos ' || '' }}${{ needs.linux-valgrind.result == 'failure' && 'linux-valgrind ' || '' }}${{ needs.linux-sanitizer.result == 'failure' && 'linux-sanitizer ' || '' }}${{ needs.random-traffic.result == 'failure' && 'random-traffic' || '' }}${{ needs.perf-nightly.result == 'failure' && ' perf-nightly' || '' }} |
There was a problem hiding this comment.
Perf failures skip Slack alert
Medium Severity
perf-nightly is wired into test-summary job-failures so nightly Slack should report perf job failures, but the benchmark run step uses continue-on-error: true, so a failed redisbench-admin run still leaves the job successful. Redisbench, module-load, and LLAPI spec failures therefore do not appear in the Slack failure list the PR aims to extend.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7e487d9. Configure here.
3 participants
Why
Two gaps, both surfaced while looking into RED-213492:
run-benchmarkPR label (benchmark-trigger.yml).JSON.SET/JSON.GET/JSON.ARRAPPEND/JSON.NUMINCRBY/JSON.NUMMULTBY. The surface RediSearch actually consumes —getAt/getArray— had no coverage, and it is not reachable over the protocol, being an in-process C ABI. So a change in the cost of an indexed read of a packed array was invisible to CI.What changed
Nightly perf run. A
perf-nightlyjob inevent-nightly.ymlcalling the existingbenchmark-runner.yml, included intest-summaryneedsand injob-failuresso a regression reaches the same Slack report as the rest of the nightly.LLAPI benchmark coverage. The LLAPI test consumer module already exposes the C API as
LLAPI.*commands, sobenchmark-flow.ymlnow builds it and passes a second--module_path(redisbench-admin's--module_pathisaction="append", and--required-module ReJSONis a presence check, so the extra module is fine). Four specs:llapi_getat_scan_packed_f32_320/_1280getAt(), the pattern a vector read uses — the pair exists so per-element cost can be compared across a 4x size jumpllapi_getat_last_packed_f32_1280llapi_getarray_packed_f32_1280The scan pair is the point: a per-element cost that grows with array length is the signal, and no absolute rps number gives you that.
LLAPI.GETAT_SCANis new in the test module (tests/pytest/llapi_test_module/module.c), with flow-test coverage intest_llapi.pyfor both the happy path and the not-an-array error path.Vectors are built by an
init_commandsEVALrather than a checked-in dataset. The values are chosen to be f32-exact but outside f16 range, so the array packs as f32 — with the obvious0.25-step values it packs as f16 instead, which is not representative of a vector workload.One builder for the test module. The
.sobuild recipe moves out oftests.shintollapi_test_module/build.sh, so the flow tests and the benchmark flow share it and cannot drift. It prints the path on stdout and sends everything else to stderr so callers can capture it.Validation
test_llapi.py: 20/20 pass (RLTest, module built from this branch, Redis 8.2), including the newtestLLAPIGetAtScanand the extendedtestLLAPIErrorsGetAt.init_commandswere run verbatim against a live server: each returnsOK, andLLAPI.GETARRAYconfirms both arrays report typef32with the expected lengths (320 / 1280).-Wall; all touched workflow files parse as YAML;bash -nclean ontests.sh.Notes for the reviewer
benchmark_globto run a subset nightly.event-tag.ymldoes not call the benchmark runner, so tagged builds still run no benchmarks; pushes to a release branch do pick upperf-nightlyvia the nightly push trigger.Generated by Claude Code
Note
Medium Risk
Nightly AWS benchmark runs add EC2 cost and a 240-minute timeout; loading the extra LLAPI module on all benchmark hosts slightly changes the environment for existing specs. No production or auth code is modified.
Overview
Schedules the existing benchmark runner as a nightly
perf-nightlyjob and folds its failures into the Slack test summary.Adds four specs that drive the shared C API (
getAt/getArray) through the LLAPI test module on packed f32 arrays (320 vs 1280), matching the RediSearch vector-read pattern.benchmark-flow.ymlnow buildsllapi_test.soand loads it beside RedisJSON on every remote run.The test module gains
LLAPI.GETAT_SCAN(with pytest coverage). Its compile recipe moves into a sharedbuild.shused by both flow tests and CI so the two cannot drift.Reviewed by Cursor Bugbot for commit 7e487d9. Bugbot is set up for automated code reviews on this repo. Configure here.