Repository navigation
chore(benchmarks): codspeed - #1253
avivkeller wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🚀 Deploying Preview to Cloudflare 🚀Preview Deployments by commit
|
7782f54 to
c8e6930
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
==========================================
- Coverage 94.18% 93.95% -0.23%
==========================================
Files 291 291
Lines 29214 29083 -131
Branches 2969 2950 -19
==========================================
- Hits 27515 27326 -189
- Misses 1677 1735 +58
Partials 22 22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| File | Main | PR | Change |
|---|---|---|---|
benchmark.json |
99.00 B | — | -99.00 B (-100.0%) |
legacy-json Generator
Output: 1 file differs
benchmark.jsonremoved
llms-txt Generator
Output size: 1 file changed · net -97.00 B
File size details
| File | Main | PR | Change |
|---|---|---|---|
benchmark.json |
97.00 B | — | -97.00 B (-100.0%) |
orama-db Generator
Output size: 1 file changed · net -97.00 B
File size details
| File | Main | PR | Change |
|---|---|---|---|
benchmark.json |
97.00 B | — | -97.00 B (-100.0%) |
web Generator
Output size: 1 file changed · net -99.00 B
File size details
| File | Main | PR | Change |
|---|---|---|---|
benchmark.json |
99.00 B | — | -99.00 B (-100.0%) |
I actually noticed a few gaps that I want to have addressed.
| ## Benchmarks | ||
|
|
||
| Generation speed and memory are measured with [CodSpeed](https://codspeed.io) | ||
| (`bench/generate.bench.mjs`), on one thread. The workflow checks `nodejs/node` |
There was a problem hiding this comment.
nit: say this is single threaded, so a flat result doesn't read as no change
| - name: Run benchmarks | ||
| uses: CodSpeedHQ/action@af75f77f17a054769abefaa42fe8dc2975469b5f # v5.6.3 | ||
| with: | ||
| mode: simulation,memory |
There was a problem hiding this comment.
Memory here is allocations on a warm build, not the peak RSS anyone actually hits. Not sure what we want those numbers to tell us
|
I also want to ensure that our benchmarking accomplishes one thing: be able to assess the impact of a PR across all possible (reasonably and measurably that make sense) points. Single threaded comparison shouldn't be the ultimate goal. Realistically no one is going to run it single threaded and bringing multi-threaded performance should matter. |
|
In a follow-up, we can set up a label based |
Right... Look, I feel that these Benchmark runs should anyways be on-demand. Not done by default. Plus it should include multi-threaded regardless. This information is pretty important IMO. And I believe we could be using Runners that support this or even shared infra we have... (ie.: self hosted runners) or something like https://www.blacksmith.sh/ IDK But TL;DR I think our Benchmark should have this from the go. I don't think it should be a follow-up (I'm not super -1 on the current PR and if you really want to push as is, I'd be fine, but I genuinely believe this should be integrated here) |
|
Requesting the walltime minutes will take time, I feel that we should start with this and work our way up |
I disagree. There's no rush here (to get this merged) and there are no other incoming performance PRs at the time except mine... (I don't consider incremental builds as a performance PR because it is a mechanism to skip building pieces... so it isn't an end-to-end verification) |
|
@nodejs/TSC I've requested the CodSpeed app to be installed on this repository |
So that we can have acurate benchmark reporting, and not the suboptimal, noisy implementation we have now