feat: CodSpeed Benchmarks #4243
Conversation
CODSPEED_FORCE_OPTIMIZATION: true
…bundled with vite
| export default defineConfig({ | ||
| plugins: [codspeedPlugin()], | ||
| // ... | ||
| }); |
There was a problem hiding this comment.
Basic config for now, will need adjustment if we decide to do testing with vitest
| IGNORED_FILES_UNPROCESSED=$(git ls-files --cached --ignored --exclude-from=all.gitignore) | ||
| IGNORED_FILES=$(grep -v -F "patches/@codspeed+core+3.1.0.patch" <<< "$IGNORED_FILES_UNPROCESSED" || true) | ||
| echo "IGNORED_FILES: $IGNORED_FILES" |
There was a problem hiding this comment.
We can revert this as soon as codspeed doesn't require the patch anymore
| diff --git a/node_modules/@codspeed/core/dist/index.cjs.js b/node_modules/@codspeed/core/dist/index.cjs.js | ||
| index 1c40cda..4a5d588 100644 | ||
| --- a/node_modules/@codspeed/core/dist/index.cjs.js | ||
| +++ b/node_modules/@codspeed/core/dist/index.cjs.js | ||
| @@ -26,7 +26,10 @@ const getV8Flags = () => { | ||
| "--no-opt", | ||
| "--predictable", | ||
| "--predictable-gc-schedule", | ||
| - "--interpreted-frames-native-stack" | ||
| + "--interpreted-frames-native-stack", | ||
| + // "--jitless", | ||
| + '--no-concurrent-sweeping', | ||
| + '--max-old-space-size=4096', | ||
| ]; | ||
| if (nodeVersionMajor < 18) { | ||
| flags.push("--no-randomize-hashes"); |
There was a problem hiding this comment.
As recommended by codspeed maintainers
| - src/__benchmarks__/github-schema.json | ||
| - src/__benchmarks__/github-schema.graphql |
There was a problem hiding this comment.
We could merge these two with the existing benchmark resource files into a shared folder
| @@ -51,14 +51,17 @@ | |||
| "build:deno": "node --loader ts-node/esm resources/build-deno.ts", | |||
| "diff:npm": "node --loader ts-node/esm resources/diff-npm-package.ts", | |||
| "gitpublish:npm": "bash ./resources/gitpublish.sh npm npmDist", | |||
| "gitpublish:deno": "bash ./resources/gitpublish.sh deno denoDist" | |||
| "gitpublish:deno": "bash ./resources/gitpublish.sh deno denoDist", | |||
| "postinstall": "patch-package" | |||
There was a problem hiding this comment.
For patching CodSpeed with added stability
|
Thanks so much for working on this, @erikwrede !!!
Just taking a quick look, this is my biggest point of concern. Is there anything strange about our benchmarks that leads to unusual variance? Is there any suggestion that we might be able to eventually migrate all benchmarks to CodSpeed? It would be great to be able to deprecate the old benchmarks entirely with a solution that provides similar coverage. |
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I can fully relate to your concerns. While I'd also love to see CodSpeed be the only benchmarking solution setup long-term, the indeterministic nature of the JIT can cause performance differences in some of the benchmarks: https://codspeed.io/erikwrede/graphql-js/benchmarks For now, I'd suggest to keep all benchmarks and just ignore all instable cases. With CodSpeed, we can freely choose our regression threshhold, but too many false positives for regressions or improvements will certainly degrade the experience. Once we see an improvement in stability over the ignored benchmarks, we can re-evaluate. My take: let's try it. If it proves unreliable, even for the benchmarks we thought to be stable, we can always remove it again and fall back to our other benchmarking solution. |
Summary
As discussed on the discord #wg channel, this is the prototype for CodSpeed benchmarks using vitest.
Some of the benchmarks we currently have may not be suitable for CodSpeeds instrumentation and may still provide variance in results. CodSpeed, for now, is just meant as to supplement the fully-fledged benchmark suite to prevent accidental regressions and get a quick impact overview on each PR. We are always able to remove certain benchmarks from the CodSpeed suite and keep them in the more powerful main benchmark suite.
Additionally, the introduction of vitest for benchmarking provides a path forward to using vitest for the tests, too
A sample run of CodSpeed on my fork can be found here: erikwrede/graphql-js#3 (comment)
Changes in this PR
@types/chaibecause Vitest bundles it, no other way I'm aware of to fix this unfortunately - no impact on developmentAdministrative steps before merging