Improve benchmark setup, consistently rely on publishConfig (#1648)

This commit has two related changes: 1. Clean up and document recent changes in the benchmark script. One of these changes is replacing ad-hoc rewrites of package.json files with a firm reliance on the publishConfig field, which should be reliable (since that's what we use when actually publishing). 2. Update all of the package.json files for published packages to have consistent publishConfig fields. This change also removed a bunch of historical cruft from the package.json files, which helped us to support environments during the ecosystem transition to package.json exports fields.
glimmerjs · Nov 3, 2024 · 57e59c4 · 57e59c4
1 parent 57d9118
commit 57e59c4
Show file tree

Hide file tree

Showing 29 changed files with 413 additions and 329 deletions.
diff --git a/.github/workflows/perf.yml b/.github/workflows/perf.yml
@@ -12,8 +12,6 @@ concurrency:
   cancel-in-progress: true
 
 env:
-  EXPERIMENT_BRANCH_NAME: ${{ github.head_ref || github.ref_name }}
-  CONTROL_BRANCH_NAME: 'main'
   FIDELITY: 100
   THROTTLE: 4
   FORK_NAME: ${{ github.event.pull_request.head.repo.full_name }}

diff --git a/.gitignore b/.gitignore
@@ -16,3 +16,4 @@ instrumentation.*.json
 .cache
 **/*.tgz
 tracerbench-results
+.rollup.cache
diff --git a/bin/published-packages.mts b/bin/published-packages.mts
@@ -0,0 +1,42 @@
+import { $ } from 'zx';
+import chalk from 'chalk';
+import { packages } from './packages.mjs';
+
+/*
+  Example JSON entry:
+
+  {
+    name: '@glimmer/validator',
+    version: '0.92.3',
+    path: '/home/ykatz/Code/Ember/glimmer-vm/packages/@glimmer/validator',
+    private: false,
+    dependencies: {
+      '@glimmer/env': [Object],
+      '@glimmer/global-context': [Object],
+      '@glimmer/interfaces': [Object],
+      '@glimmer/util': [Object]
+    },
+    devDependencies: {
+      '@glimmer-workspace/build-support': [Object],
+      '@glimmer/debug-util': [Object],
+      '@glimmer/local-debug-flags': [Object],
+      eslint: [Object],
+      publint: [Object],
+      rollup: [Object],
+      typescript: [Object]
+    }
+  }
+*/
+
+/**
+ * @typedef {} PackageEntry
+ */
+
+const entries = await packages('@glimmer');
+
+const quiet = process.argv.includes('--quiet') || process.argv.includes('-q');
+
+for (const entry of entries) {
+  console.log(entry.name);
+  if (!quiet) console.error(chalk.gray(`  ${entry.path}`));
+}
diff --git a/bin/setup-bench.mjs b/bin/setup-bench.mjs
@@ -1,10 +1,11 @@
 import 'zx/globals';
 import os from 'node:os';
 import { join } from 'node:path';
-import chalk from 'chalk';
+import { readFile, writeFile } from 'node:fs/promises';
 
 const ROOT = new URL('..', import.meta.url).pathname;
 $.verbose = true;
+
 const REUSE_CONTROL = !!process.env['REUSE_CONTROL'];
 
 /*
@@ -56,7 +57,6 @@ const markers = (process.env['MARKERS'] || appMarkers)
   .join(',');
 const fidelity = process.env['FIDELITY'] || '20';
 const throttleRate = process.env['THROTTLE'] || '2';
-const FORK_NAME = process.env['FORK_NAME'] || '';
 
 const tempDir = os.tmpdir();
 
@@ -84,81 +84,51 @@ if (!REUSE_CONTROL) {
 await $`rm -rf ${EXPERIMENT_DIR}`;
 await $`mkdir ${EXPERIMENT_DIR}`;
 
-const isMacOs = os.platform() === 'darwin';
-
+// Intentionally use the same folder for both experiment and control to make it easier to
+// make changes to the benchmark suite itself and compare the results.
 const BENCHMARK_FOLDER = join(pwd, benchmarkFolder);
 
 const CONTROL_PORT = 4020;
 const EXPERIMENT_PORT = 4021;
 const CONTROL_URL = `http://localhost:${CONTROL_PORT}`;
 const EXPERIMENT_URL = `http://localhost:${EXPERIMENT_PORT}`;
 
-// we can't do it in parallel on CI,
-
-if (!REUSE_CONTROL) {
-  // setup control
-  await within(async () => {
-    await $`git fetch origin`;
-    const mainRef = await $`git rev-parse origin/main`;
-    await cd(CONTROL_DIR);
-    await $`git clone ${join(ROOT, '.git')} .`;
-    await $`git reset --hard ${mainRef}`;
-    await $`rm -rf ./benchmark`;
-    await $`cp -r ${BENCHMARK_FOLDER} ./benchmark`;
-
-    console.info(`$ pnpm install --no-frozen-lockfile ${chalk.gray('[control]')}`);
-
-    await $`pwd`;
-    const result = await $`pnpm install`;
-    console.log(result);
-
-    console.info(`$ pnpm build ${chalk.gray('[control]')}`);
-
-    await $`pnpm build`;
-
-    if (isMacOs) {
-      await $`find ./packages -name 'package.json' -exec sed -i '' 's|"main": "index.ts",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-      await $`find ./packages -name 'package.json' -exec sed -i '' 's|"main": "./dist/index.js",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-      await $`find ./packages -name 'package.json' -exec sed -i '' 's|"import": "./dist/index.js"|"import": "./dist/prod/index.js"|g' {} \\;`;
-    } else {
-      await $`find ./packages -name 'package.json' -exec sed -i 's|"main": "index.ts",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-      await $`find ./packages -name 'package.json' -exec sed -i 's|"main": "./dist/index.js",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-      await $`find ./packages -name 'package.json' -exec sed -i 's|"import": "./dist/index.js"|"import": "./dist/prod/index.js"|g' {} \\;`;
-    }
+// make sure that the origin is up to date so we get the right control
+await $`git fetch origin`;
 
-    await cd(CONTROL_BENCH_DIR);
-    await $`pnpm vite build`;
-  });
-}
+// now we can get the ref of the control branch so we can check it out later
+const controlRef = (await $`git rev-parse origin/main`).stdout.trim();
 
-// setup experiment
-await within(async () => {
-  await cd(EXPERIMENT_DIR);
-  await $`git clone ${join(ROOT, '.git')} .`;
-  await $`git checkout --force ${experimentRef}`;
-  await $`rm -rf ./benchmark`;
-  await $`cp -r ${BENCHMARK_FOLDER} ./benchmark`;
+// we can't do it in parallel on CI,
 
-  console.info(`$ pnpm install --no-frozen-lockfile ${chalk.gray('[experiment]')}`);
-  const install = () => $`pnpm install --no-frozen-lockfile`.pipe(process.stderr);
-  await spinner(install);
-  console.info(`$ pnpm build ${chalk.gray('[experiment]')}`);
-  const build = () => $`pnpm build`.pipe(process.stderr);
-  await spinner(build);
-
-  if (isMacOs) {
-    await $`find ./packages -name 'package.json' -exec sed -i '' 's|"main": "index.ts",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-    await $`find ./packages -name 'package.json' -exec sed -i '' 's|"main": "./dist/index.js",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-    await $`find ./packages -name 'package.json' -exec sed -i '' 's|"import": "./dist/index.js"|"import": "./dist/prod/index.js"|g' {} \\;`;
-  } else {
-    await $`find ./packages -name 'package.json' -exec sed -i 's|"main": "index.ts",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-    await $`find ./packages -name 'package.json' -exec sed -i 's|"main": "./dist/index.js",|"main": "./dist/prod/index.js","module": "./dist/prod/index.js",|g' {} \\;`;
-    await $`find ./packages -name 'package.json' -exec sed -i 's|"import": "./dist/index.js"|"import": "./dist/prod/index.js"|g' {} \\;`;
+/**
+ * Rewrite all `package.json`s with a `publishConfig` field with the fields specified in
+ * `publishConfig`.
+ */
+async function rewritePackageJson() {
+  // limit to `@glimmer/*` packages
+  const packages = await $`find ./packages/@glimmer -name 'package.json'`;
+
+  for (const pkg of packages.stdout.trim().split('\n')) {
+    const packageJson = JSON.parse(await readFile(pkg, { encoding: 'utf8' }));
+    const publishConfig = packageJson['publishConfig'];
+
+    // assume that the presence of a `publishConfig` field means that the package is
+    // a published package and needs its package.json updated to behave like a published
+    // package in the benchmark environment.
+    if (publishConfig) {
+      const updatedPkg = { ...packageJson, ...publishConfig };
+
+      for (const [key, value] of Object.entries(publishConfig)) {
+        if (value === null) {
+          delete updatedPkg[key];
+        }
+      }
+
+      await writeFile(pkg, JSON.stringify(updatedPkg, null, 2), { encoding: 'utf8' });
+    }
   }
-
-  await cd(EXPERIMENT_BENCH_DIR);
-  await $`pnpm vite build`;
-});
+}
 
 console.info({
   control: controlBranchName,
@@ -167,6 +137,18 @@ console.info({
   CONTROL_DIR,
 });
 
+// setup experiment
+await within(async () => {
+  await buildRepo(EXPERIMENT_DIR, experimentRef);
+});
+
+if (!REUSE_CONTROL) {
+  // setup control
+  await within(async () => {
+    await buildRepo(CONTROL_DIR, controlRef);
+  });
+}
+
 // start build assets
 $`cd ${CONTROL_BENCH_DIR} && pnpm vite preview --port ${CONTROL_PORT}`;
 $`cd ${EXPERIMENT_BENCH_DIR} && pnpm vite preview --port ${EXPERIMENT_PORT}`;
@@ -194,3 +176,40 @@ try {
 }
 
 process.exit(0);
+
+/**
+ * @param {string} directory the directory to clone into
+ * @param {string} ref the ref to checkout
+ */
+async function buildRepo(directory, ref) {
+  // the benchmark directory is located in `packages/@glimmer/benchmark` in each of the
+  // experiment and control checkouts
+  const benchDir = join(directory, 'benchmark', 'benchmarks', 'krausest');
+
+  await cd(directory);
+
+  // write the `pwd` to the output to make it easier to debug if something goes wrong
+  await $`pwd`;
+
+  // clone the raw git repo for the experiment
+  await $`git clone ${join(ROOT, '.git')} .`;
+
+  // checkout the repo to the HEAD of the current branch
+  await $`git checkout --force ${ref}`;
+
+  // recreate the benchmark directory
+  await $`rm -rf ./benchmark`;
+  // intentionally use the same folder for both experiment and control
+  await $`cp -r ${BENCHMARK_FOLDER} ./benchmark`;
+
+  // `pnpm install` and build the repo
+  await $`pnpm install --no-frozen-lockfile`;
+  await $`pnpm build`;
+
+  // rewrite all `package.json`s to behave like published packages
+  await rewritePackageJson();
+
+  // build the benchmarks using vite
+  await cd(benchDir);
+  await $`pnpm vite build`;
+}
diff --git a/bin/tsconfig.json b/bin/tsconfig.json
@@ -2,18 +2,15 @@
   "compilerOptions": {
     "composite": true,
     "skipLibCheck": true,
-
     "baseUrl": ".",
     "allowJs": true,
     "checkJs": true,
     "outDir": "../ts-dist/bin",
-
     "target": "es2020",
     "module": "esnext",
     "moduleResolution": "bundler",
     "allowSyntheticDefaultImports": true,
     "verbatimModuleSyntax": true,
-
     "strict": true,
     "suppressImplicitAnyIndexErrors": false,
     "useDefineForClassFields": false,

diff --git a/packages/@glimmer-workspace/benchmark-env/package.json b/packages/@glimmer-workspace/benchmark-env/package.json
@@ -3,6 +3,7 @@
   "version": "0.84.3",
   "private": true,
   "repository": "https://github.com/glimmerjs/glimmer-vm/tree/main/packages/@glimmer-workspace/benchmark-env",
+  "type": "module",
   "main": "index.ts",
   "scripts": {
     "test:lint": "eslint .",

diff --git a/packages/@glimmer-workspace/integration-tests/test/chaos-rehydration-test.ts b/packages/@glimmer-workspace/integration-tests/test/chaos-rehydration-test.ts
@@ -89,11 +89,10 @@ abstract class AbstractChaosMonkeyTest extends RenderTest {
 
     let removedNodeDisplay: Nullable<string>;
     switch (nodeToRemove.nodeType) {
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
       case COMMENT_NODE:
         removedNodeDisplay = `<!--${nodeToRemove.nodeValue}-->`;
         break;
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
+
       case ELEMENT_NODE:
         removedNodeDisplay = castToBrowser(nodeToRemove, ['HTML', 'SVG']).outerHTML;
         break;

diff --git a/packages/@glimmer/compiler/package.json b/packages/@glimmer/compiler/package.json
@@ -4,29 +4,29 @@
   "license": "MIT",
   "repository": "https://github.com/glimmerjs/glimmer-vm/tree/main/packages/@glimmer/compiler",
   "type": "module",
-  "main": "index.ts",
-  "types": "index.ts",
-  "exports": {
-    "types": "./index.ts",
-    "development": "./index.ts",
-    "import": "./dist/index.js"
-  },
+  "exports": "./index.ts",
   "publishConfig": {
     "access": "public",
-    "types": "dist/dev/index.d.ts",
     "exports": {
       ".": {
-        "types": "./dist/dev/index.d.ts",
-        "development": {
-          "require": "./dist/dev/index.cjs",
-          "import": "./dist/dev/index.js"
+        "require": {
+          "development": {
+            "types": "./dist/dev/index.d.cts",
+            "default": "./dist/dev/index.cjs"
+          }
         },
-        "require": "./dist/dev/index.cjs",
-        "import": "./dist/dev/index.js"
+        "default": {
+          "development": {
+            "types": "./dist/dev/index.d.ts",
+            "default": "./dist/dev/index.js"
+          },
+          "default": {
+            "types": "./dist/prod/index.d.ts",
+            "default": "./dist/prod/index.js"
+          }
+        }
       }
-    },
-    "main": null,
-    "module": "dist/dev/index.js"
+    }
   },
   "files": [
     "dist"

diff --git a/packages/@glimmer/debug-util/package.json b/packages/@glimmer/debug-util/package.json
@@ -6,32 +6,9 @@
   "description": "Common utilities used in Glimmer with debug-specific behavior",
   "repository": "https://github.com/tildeio/glimmer/tree/main/packages/@glimmer/debug-util",
   "type": "module",
-  "main": "index.ts",
-  "types": "index.ts",
-  "publishConfig": {
-    "access": "public",
-    "types": "dist/dev/index.d.ts",
-    "exports": {
-      ".": {
-        "types": "./dist/dev/index.d.ts",
-        "development": {
-          "require": "./dist/dev/index.cjs",
-          "import": "./dist/dev/index.js"
-        },
-        "require": "./dist/dev/index.cjs",
-        "import": "./dist/prod/index.js"
-      }
-    },
-    "main": null,
-    "module": "dist/dev/index.js"
-  },
-  "files": [
-    "dist"
-  ],
+  "exports": "./index.ts",
   "scripts": {
-    "build": "rollup -c rollup.config.mjs",
     "test:lint": "eslint .",
-    "test:publint": "publint",
     "test:types": "tsc --noEmit -p ../tsconfig.json"
   },
   "dependencies": {