-
Notifications
You must be signed in to change notification settings - Fork 470
chore: track download size per PR in the benchmark comment #8455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| /* | ||
| * Measures the size impact of a change and writes the numbers as `.delta.*` files for | ||
| * `netlify/delta-action` to compare against `main` and post on the PR. See | ||
| * `.github/workflows/benchmark.yml`. | ||
| */ | ||
|
|
||
| import { execFile } from 'node:child_process' | ||
| import { readdir, readFile, lstat, mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises' | ||
| import { tmpdir } from 'node:os' | ||
| import path from 'node:path' | ||
| import process from 'node:process' | ||
| import { fileURLToPath, pathToFileURL } from 'node:url' | ||
| import { promisify } from 'node:util' | ||
|
|
||
| const execFileAsync = promisify(execFile) | ||
|
|
||
| /** cacache stores downloaded tarballs under this key prefix; registry metadata shares the prefix. */ | ||
| const TARBALL_KEY_SUFFIX = '.tgz' | ||
|
|
||
| const walkFiles = async (dir) => { | ||
| let entries | ||
| try { | ||
| entries = await readdir(dir, { withFileTypes: true }) | ||
| } catch (error) { | ||
| if (error.code === 'ENOENT' || error.code === 'ENOTDIR') { | ||
| return [] | ||
| } | ||
| throw error | ||
| } | ||
|
|
||
| const nested = await Promise.all( | ||
| entries.map(async (entry) => { | ||
| const entryPath = path.join(dir, entry.name) | ||
| return entry.isDirectory() ? walkFiles(entryPath) : [entryPath] | ||
| }), | ||
| ) | ||
| return nested.flat() | ||
| } | ||
|
|
||
| /** | ||
| * Total bytes of every package tarball npm downloaded into `cacheDir`. | ||
| * | ||
| * Read from cacache's index rather than by measuring the cache on disk: the index records an exact | ||
| * byte count per entry, and lets us exclude cached registry metadata, which is downloaded too but | ||
| * fluctuates as unrelated packages publish. Tarballs for a published version are immutable, so a | ||
| * given lockfile always produces the same total. | ||
| */ | ||
| export const sumCachedTarballBytes = async (cacheDir) => { | ||
| const buckets = await walkFiles(path.join(cacheDir, '_cacache', 'index-v5')) | ||
|
|
||
| // Buckets are append-only logs, so one key can appear several times. Keep the last entry per key | ||
| // rather than summing every line, which would count a re-fetched tarball more than once. | ||
| const sizeByKey = new Map() | ||
|
|
||
| for (const bucket of buckets) { | ||
| const contents = await readFile(bucket, 'utf8') | ||
| for (const line of contents.split('\n')) { | ||
| const separator = line.indexOf('\t') | ||
| if (separator === -1) { | ||
| continue | ||
| } | ||
| let entry | ||
| try { | ||
| entry = JSON.parse(line.slice(separator + 1)) | ||
| } catch { | ||
| // A partially written line is normal in an append-only log. | ||
| continue | ||
| } | ||
| if (entry?.key?.endsWith(TARBALL_KEY_SUFFIX) && typeof entry.size === 'number') { | ||
| sizeByKey.set(entry.key, entry.size) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return [...sizeByKey.values()].reduce((total, size) => total + size, 0) | ||
| } | ||
|
|
||
| /** | ||
| * Total bytes of the files under `dir`. | ||
| * | ||
| * Uses real file sizes rather than `du`, which reports disk blocks and so inflates a tree of many | ||
| * small files by an amount that varies with the filesystem. Symlinks are measured as links, not | ||
| * followed, so `node_modules/.bin` doesn't count binaries twice. | ||
| */ | ||
| export const directoryBytes = async (dir) => { | ||
| const files = await walkFiles(dir) | ||
| const sizes = await Promise.all( | ||
| files.map(async (file) => { | ||
| try { | ||
| return (await lstat(file)).size | ||
| } catch { | ||
| return 0 | ||
| } | ||
| }), | ||
| ) | ||
| return sizes.reduce((total, size) => total + size, 0) | ||
| } | ||
|
|
||
| /** Renders one metric in the two-line shape `delta-action` parses: value, then `unit (label)`. */ | ||
| export const formatDelta = (value, unit, label) => `${Math.round(value).toString()}\n${unit} (${label})\n` | ||
|
|
||
| /** Counts installed packages by looking for the manifests npm wrote, including nested copies. */ | ||
| const countPackages = async (nodeModulesDir) => { | ||
| const files = await walkFiles(nodeModulesDir) | ||
| return files.filter((file) => path.basename(file) === 'package.json').length | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- applicable repository guidance ---'
head -5 /tmp/coderabbit-repo-knowledge/netlify-cli-013cd098/*/*.md 2>/dev/null || true
printf '%s\n' '--- changed hunk ---'
git diff --unified=35 -- scripts/measure-size.js
printf '%s\n' '--- relevant source ---'
cat -n scripts/measure-size.js | sed -n '1,180p'
printf '%s\n' '--- package-count callers and related install/lockfile logic ---'
rg -n -C 5 'countPackages|dependencyCount|package-lock|nodeModulesDir|installDir|walkFiles' scripts/measure-size.jsRepository: netlify/cli Length of output: 14665 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- package metadata and lockfile format ---'
for f in package.json package-lock.json npm-shrinkwrap.json; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,80p' "$f"
fi
done
printf '%s\n' '--- measure-size references and tests ---'
rg -n -C 8 'measure-size|installedPackageCount|dependencyCount|countPackages' --glob '!scripts/measure-size.js' .
printf '%s\n' '--- package manifests in tracked dependency fixtures/examples ---'
rg -l --glob 'package.json' --glob '!package.json' --glob '!node_modules/**' . | head -80
printf '%s\n' '--- remaining script ---'
cat -n scripts/measure-size.js | sed -n '175,230p'Repository: netlify/cli Length of output: 7387 Count installed package roots only. 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /** | ||
| * Builds the tarball a release would publish, installs it the way a user would, and reports what | ||
| * that cost. Measuring a real install rather than the repo's own `node_modules` is what lets these | ||
| * numbers cover our own package contents as well as our dependencies. | ||
| */ | ||
| const measure = async (repoDir, scratchDir) => { | ||
| const packDir = path.join(scratchDir, 'pack') | ||
| const installDir = path.join(scratchDir, 'install') | ||
| const cacheDir = path.join(scratchDir, 'npm-cache') | ||
| await mkdir(packDir, { recursive: true }) | ||
| await mkdir(installDir, { recursive: true }) | ||
|
|
||
| const { stdout: packStdout } = await execFileAsync( | ||
| 'npm', | ||
| ['pack', '--json', '--pack-destination', packDir, '--silent'], | ||
| { cwd: repoDir, maxBuffer: 64 * 1024 * 1024 }, | ||
| ) | ||
| const [packed] = JSON.parse(packStdout) | ||
| const tarballPath = path.join(packDir, packed.filename) | ||
|
|
||
| await writeFile( | ||
| path.join(installDir, 'package.json'), | ||
| `${JSON.stringify({ name: 'size-probe', version: '1.0.0', private: true }, null, 2)}\n`, | ||
| ) | ||
|
|
||
| // `--ignore-scripts` keeps this deterministic and safe; it means the total is what npm downloads, | ||
| // not what a dependency's own postinstall might fetch afterwards. | ||
| await execFileAsync( | ||
| 'npm', | ||
| [ | ||
| 'install', | ||
| tarballPath, | ||
| '--omit=dev', | ||
| '--ignore-scripts', | ||
| '--no-audit', | ||
| '--no-fund', | ||
| '--cache', | ||
| cacheDir, | ||
| '--loglevel', | ||
| 'error', | ||
| ], | ||
| { cwd: installDir, maxBuffer: 64 * 1024 * 1024 }, | ||
| ) | ||
|
|
||
| const nodeModulesDir = path.join(installDir, 'node_modules') | ||
| // The CLI tarball is installed from disk, so it never lands in the cache -- add it back to get | ||
| // the full download a user would perform. | ||
| const dependencyTarballBytes = await sumCachedTarballBytes(cacheDir) | ||
|
|
||
| return { | ||
| packageDownloadBytes: packed.size, | ||
| totalDownloadBytes: dependencyTarballBytes + packed.size, | ||
| installedBytes: await directoryBytes(nodeModulesDir), | ||
| dependencyCount: await countPackages(nodeModulesDir), | ||
| } | ||
| } | ||
|
|
||
| const main = async () => { | ||
| const repoDir = path.join(path.dirname(fileURLToPath(import.meta.url)), '..') | ||
| const scratchDir = await mkdtemp(path.join(process.env.RUNNER_TEMP ?? tmpdir(), 'measure-size-')) | ||
|
|
||
| try { | ||
| const result = await measure(repoDir, scratchDir) | ||
|
|
||
| const metrics = [ | ||
| ['.delta.downloadSizePackage', result.packageDownloadBytes / 1024, 'kb', 'Download size (CLI package)'], | ||
| ['.delta.downloadSizeInstall', result.totalDownloadBytes / 1024, 'kb', 'Download size (full install)'], | ||
| ['.delta.installedSize', result.installedBytes / 1024, 'kb', 'Installed size'], | ||
| // Deliberately not `.delta.dependencyCount`: this counts every package directory npm wrote, | ||
| // including nested duplicate copies, so it is a different measurement from the `npm ls` count | ||
| // that key used to hold. Reusing the key would compare the two and report a phantom jump. | ||
|
Comment on lines
+176
to
+178
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Remove the behavior comments. These lines explain implementation behavior. The metric name and label already describe the output. Remove the comments and keep the metric entry unchanged. Proposed fix- // Deliberately not `.delta.dependencyCount`: this counts every package directory npm wrote,
- // including nested duplicate copies, so it is a different measurement from the `npm ls` count
- // that key used to hold. Reusing the key would compare the two and report a phantom jump.
['.delta.installedPackageCount', result.dependencyCount, '', 'Installed package count'],As per coding guidelines, 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| ['.delta.installedPackageCount', result.dependencyCount, '', 'Installed package count'], | ||
| ] | ||
|
|
||
| for (const [filename, value, unit, label] of metrics) { | ||
| await writeFile(path.join(repoDir, filename), formatDelta(value, unit, label)) | ||
| console.log(`${label}: ${Math.round(value).toLocaleString()} ${unit}`.trim()) | ||
| } | ||
| } finally { | ||
| await rm(scratchDir, { recursive: true, force: true }) | ||
| } | ||
| } | ||
|
|
||
| if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { | ||
| await main() | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,120 @@ | ||||||||||||||||||||||
| import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises' | ||||||||||||||||||||||
| import { tmpdir } from 'node:os' | ||||||||||||||||||||||
| import path from 'node:path' | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import { afterEach, beforeEach, describe, expect, test } from 'vitest' | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import { directoryBytes, formatDelta, sumCachedTarballBytes } from '../../../scripts/measure-size.js' | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| let workDir: string | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| beforeEach(async () => { | ||||||||||||||||||||||
| workDir = await mkdtemp(path.join(tmpdir(), 'measure-size-')) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| afterEach(async () => { | ||||||||||||||||||||||
| await rm(workDir, { recursive: true, force: true }) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Writes a cacache index bucket. Real buckets are append-only logs of `<hash>\t<json>` lines, which | ||||||||||||||||||||||
| * is the detail most of these tests exist to pin down. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| const writeIndexBucket = async (cacheDir: string, bucketName: string, entries: object[]) => { | ||||||||||||||||||||||
| const bucketDir = path.join(cacheDir, '_cacache', 'index-v5', 'aa', 'bb') | ||||||||||||||||||||||
| await mkdir(bucketDir, { recursive: true }) | ||||||||||||||||||||||
| const contents = entries.map((entry) => `0000000000\t${JSON.stringify(entry)}`).join('\n') | ||||||||||||||||||||||
| await writeFile(path.join(bucketDir, bucketName), `${contents}\n`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const tarballEntry = (name: string, version: string, size: number) => ({ | ||||||||||||||||||||||
| key: `make-fetch-happen:request-cache:https://registry.npmjs.org/${name}/-/${name}-${version}.tgz`, | ||||||||||||||||||||||
| integrity: 'sha512-fake', | ||||||||||||||||||||||
| time: 1_770_825_313_018, | ||||||||||||||||||||||
| size, | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| describe('sumCachedTarballBytes', () => { | ||||||||||||||||||||||
| test('sums the byte size of every cached tarball', async () => { | ||||||||||||||||||||||
| await writeIndexBucket(workDir, 'bucket1', [tarballEntry('chalk', '5.3.0', 13_397)]) | ||||||||||||||||||||||
| await writeIndexBucket(workDir, 'bucket2', [tarballEntry('diff', '4.0.4', 98_055)]) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(await sumCachedTarballBytes(workDir)).toBe(111_452) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('ignores cached registry metadata, counting only tarballs', async () => { | ||||||||||||||||||||||
| await writeIndexBucket(workDir, 'bucket1', [ | ||||||||||||||||||||||
| tarballEntry('chalk', '5.3.0', 13_397), | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| key: 'make-fetch-happen:request-cache:https://registry.npmjs.org/chalk', | ||||||||||||||||||||||
| integrity: 'sha512-fake', | ||||||||||||||||||||||
| time: 1_770_825_313_018, | ||||||||||||||||||||||
| size: 5_000_000, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| ]) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(await sumCachedTarballBytes(workDir)).toBe(13_397) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('counts a re-fetched tarball once rather than once per log entry', async () => { | ||||||||||||||||||||||
| // cacache appends rather than rewrites, so the same key legitimately appears more than once. | ||||||||||||||||||||||
| await writeIndexBucket(workDir, 'bucket1', [ | ||||||||||||||||||||||
| tarballEntry('chalk', '5.3.0', 13_397), | ||||||||||||||||||||||
| tarballEntry('chalk', '5.3.0', 13_397), | ||||||||||||||||||||||
| ]) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(await sumCachedTarballBytes(workDir)).toBe(13_397) | ||||||||||||||||||||||
|
Comment on lines
+62
to
+66
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Make this test verify last-entry semantics. The helper in Proposed test adjustment tarballEntry('chalk', '5.3.0', 13_397),
- tarballEntry('chalk', '5.3.0', 13_397),
+ tarballEntry('chalk', '5.3.0', 14_001),
])
- expect(await sumCachedTarballBytes(workDir)).toBe(13_397)
+ expect(await sumCachedTarballBytes(workDir)).toBe(14_001)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('skips unparseable lines rather than throwing', async () => { | ||||||||||||||||||||||
| const bucketDir = path.join(workDir, '_cacache', 'index-v5', 'aa', 'bb') | ||||||||||||||||||||||
| await mkdir(bucketDir, { recursive: true }) | ||||||||||||||||||||||
| await writeFile( | ||||||||||||||||||||||
| path.join(bucketDir, 'bucket1'), | ||||||||||||||||||||||
| `0000000000\t{"truncated":\n0000000000\t${JSON.stringify(tarballEntry('chalk', '5.3.0', 13_397))}\n`, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(await sumCachedTarballBytes(workDir)).toBe(13_397) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('returns zero when nothing was downloaded', async () => { | ||||||||||||||||||||||
| expect(await sumCachedTarballBytes(path.join(workDir, 'never-created'))).toBe(0) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| describe('directoryBytes', () => { | ||||||||||||||||||||||
| test('sums real file sizes across nested directories', async () => { | ||||||||||||||||||||||
| await mkdir(path.join(workDir, 'nested'), { recursive: true }) | ||||||||||||||||||||||
| await writeFile(path.join(workDir, 'a.js'), 'x'.repeat(100)) | ||||||||||||||||||||||
| await writeFile(path.join(workDir, 'nested', 'b.js'), 'x'.repeat(250)) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(await directoryBytes(workDir)).toBe(350) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('counts a symlink itself rather than following it to its target', async () => { | ||||||||||||||||||||||
| // `node_modules/.bin` is full of symlinks; following them would count binaries repeatedly. | ||||||||||||||||||||||
| const { symlink } = await import('node:fs/promises') | ||||||||||||||||||||||
| await writeFile(path.join(workDir, 'real.js'), 'x'.repeat(100)) | ||||||||||||||||||||||
| await symlink(path.join(workDir, 'real.js'), path.join(workDir, 'link.js')) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(await directoryBytes(workDir)).toBeLessThan(200) | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Assert the symlink's own size.
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('returns zero for a directory that does not exist', async () => { | ||||||||||||||||||||||
| expect(await directoryBytes(path.join(workDir, 'never-created'))).toBe(0) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| describe('formatDelta', () => { | ||||||||||||||||||||||
| test('renders the value and label in the format delta-action parses', () => { | ||||||||||||||||||||||
| expect(formatDelta(1234, 'kb', 'Download size (full install)')).toBe('1234\nkb (Download size (full install))\n') | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('omits the unit for unitless metrics, keeping the leading space', () => { | ||||||||||||||||||||||
| expect(formatDelta(1093, '', 'Dependency count')).toBe('1093\n (Dependency count)\n') | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('rounds fractional values, since delta-action expects an integer', () => { | ||||||||||||||||||||||
| expect(formatDelta(1234.6, 'kb', 'Installed size')).toBe('1235\nkb (Installed size)\n') | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove behavior comments from
scripts/measure-size.js.Use function names, identifiers, and structure that state the behavior without explanatory comments.
scripts/measure-size.js#L1-L5: remove the file-header behavior description.scripts/measure-size.js#L17-L17: remove the constant behavior description.scripts/measure-size.js#L40-L47: remove the cache-byte behavior description.scripts/measure-size.js#L51-L52: remove the cache deduplication behavior description.scripts/measure-size.js#L66-L66: remove the malformed-entry behavior description.scripts/measure-size.js#L78-L84: remove the directory-size behavior description.scripts/measure-size.js#L99-L99: remove the metric-format behavior description.scripts/measure-size.js#L102-L102: remove the package-count behavior description.scripts/measure-size.js#L108-L112: remove the measurement behavior description.scripts/measure-size.js#L133-L134: remove the install-option behavior description.scripts/measure-size.js#L153-L154: remove the download-total behavior description.As per coding guidelines,
**/*.{js,jsx,ts,tsx,mjs,cjs,go,rs}: “Never write comments on what the code does, make the code clean and self explanatory instead.”📍 Affects 1 file
scripts/measure-size.js#L1-L5(this comment)scripts/measure-size.js#L17-L17scripts/measure-size.js#L40-L47scripts/measure-size.js#L51-L52scripts/measure-size.js#L66-L66scripts/measure-size.js#L78-L84scripts/measure-size.js#L99-L99scripts/measure-size.js#L102-L102scripts/measure-size.js#L108-L112scripts/measure-size.js#L133-L134scripts/measure-size.js#L153-L154🤖 Prompt for AI Agents
Source: Coding guidelines