[benchmark] lock file resolution#14764
Conversation
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This adds a fixtures/lockfile-resolution-benchmark fixture that spawns the workspace Wrangler and a pkg.pr.new prerelease from #14703 to compare dry-run deploy timings.
Ranked issues:
- (medium) The benchmark's
test:ciwires into the main CI graph on all 3 OSes, permanently pulling apkg.pr.newprerelease and running ~12 wrangler spawns per CI run — surprising for a one-off measurement PR. - (low)
summarizecomputes the median incorrectly for even-length inputs; safe attrials = 5today but silently wrong iftrialsis bumped to an even number.
Details inline.
| const sorted = values.toSorted((a, b) => a - b); | ||
| return { | ||
| min: sorted[0], | ||
| median: sorted[Math.floor(sorted.length / 2)], |
There was a problem hiding this comment.
median only picks a single element, so for an even number of trials it returns the upper-middle value rather than the average of the two middle values. Harmless while trials = 5 (odd), but the whole point of this fixture is accurate stats, so it's worth making it robust in case trials changes:
| median: sorted[Math.floor(sorted.length / 2)], | |
| median: | |
| sorted.length % 2 === 0 | |
| ? (sorted[sorted.length / 2 - 1] + sorted[sorted.length / 2]) / 2 | |
| : sorted[Math.floor(sorted.length / 2)], |
| [#14703](https://github.com/cloudflare/workers-sdk/pull/14703). | ||
|
|
||
| Its `test:ci` script warms both CLIs, runs five alternating trials, and prints a | ||
| single timing summary. The fixture test suite runs automatically on Linux, |
There was a problem hiding this comment.
This documents that the benchmark runs automatically in the main CI workflow on all three OSes. Because test:ci is the standard CI task, every CI run will now fetch the pkg.pr.new prerelease (wrangler-pr) and spawn wrangler ~12 times on Linux/macOS/Windows. That's a real recurring cost and an external network dependency (pkg.pr.new) baked into pnpm-lock.yaml/the catalog. Given the [benchmark] title and the exploratory PR body, this looks like a one-off measurement that shouldn't be merged into main permanently — consider dropping it before merge, or gating the script so it's opt-in (e.g. behind an env flag) rather than part of the default CI graph.
|
Review posted successfully to PR #14764. Summary of what I flagged:
|
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
2 participants
Fixes #[insert GH or internal issue link(s)].
Trying to get a sense of the additional cost for resolving from lockfile in #14703
A picture of a cute animal (not mandatory, but encouraged)