Fix benchmark debug action (#4187)

* Fix benchmark debug action

* Add parameter to analyze to specify which benchmark logs to analyze

* Specify benchmark to analyze in benchmark worker workflow

* Make analyze resilient to malformed logs

.github/workflows/ci.yml

@@ -8,7 +8,7 @@ on:
         description: 'Branch or tag ref to check out'
         type: string
         required: false
-        default: 'main'
+        default: ''
       artifact_name:
         description: 'Name of the artifact to upload'
         type: string

.github/workflows/run-bench.yml

@@ -53,6 +53,9 @@ jobs:
         with:
           name: results
           path: benches/results/${{ inputs.benchmark }}.json
+      - name: Anaylze logs if present
+        working-directory: benches
+        run: '[ -d logs ] && npm run analyze ${{ inputs.benchmark }} || echo "No logs to analyze"'
       - name: Tar logs if present
         working-directory: benches
         run: '[ -d logs ] && tar -zcvf ${{ inputs.benchmark}}_logs.tgz logs || echo "No logs found"'

.github/workflows/single-bench.yml

@@ -26,11 +26,15 @@ on:
         type: boolean
         default: true
         required: false
-      timeout:
-        description: 'How many minutes to give the benchmark to run before timing out and failing'
-        type: number
-        default: 20
-        required: false
+      # A bug in GitHub actions prevents us from passing numbers (as either
+      # number or string type) to called workflows. So disabling this for now.
+      # See: https://github.com/orgs/community/discussions/67182
+      #
+      # timeout:
+      #   description: 'How many minutes to give the benchmark to run before timing out and failing'
+      #   type: number
+      #   default: 20
+      #   required: false
 
 jobs:
   build_local:
@@ -57,17 +61,17 @@ jobs:
         with:
           name: npm-package
       - run: mv preact.tgz preact-local.tgz
+      - name: Upload locally built preact package
+        uses: actions/upload-artifact@v3
+        with:
+          name: bench-environment
+          path: preact-local.tgz
       - name: Clear working directory
         run: |
           ls -al
           rm -rf *
           echo "===================="
           ls -al
-      - name: Upload locally built preact package
-        uses: actions/upload-artifact@v3
-        with:
-          name: bench-environment
-          path: preact-local.tgz
       - name: Download base package
         uses: actions/download-artifact@v3
         with:
@@ -80,10 +84,10 @@ jobs:
           path: preact-main.tgz
 
   benchmark:
-    name: Bench ${{ inputs.benchmark}}
+    name: Bench ${{ inputs.benchmark }}
     uses: ./.github/workflows/run-bench.yml
     needs: prepare
     with:
-      benchmark: ${{ inputs.benchmark}}
-      timeout: ${{ inputs.timeout }}
+      benchmark: ${{ inputs.benchmark }}
       trace: ${{ inputs.trace }}
+      # timeout: ${{ inputs.timeout }}

benches/scripts/analyze.js

@@ -87,7 +87,7 @@ function setInMapArray(map, key, index, value) {
 function logDifferences(key, results) {
 	let withDifferences = computeDifferences(results);
 	console.log();
-	let { fixed, unfixed } = automaticResultTable(withDifferences);
+	let { unfixed } = automaticResultTable(withDifferences);
 	// console.log(horizontalTermResultTable(fixed));
 	console.log(key);
 	console.log(verticalTermResultTable(unfixed));
@@ -96,7 +96,7 @@ function logDifferences(key, results) {
 /**
  * @param {string} version
  * @param {string[]} logPaths
- * @param {(logs: TraceEvent[], logFilePath: string) => number} [getThreadId]
+ * @param {(logs: TraceEvent[], logFilePath: string) => number | null} [getThreadId]
  * @param {(log: TraceEvent) => boolean} [trackEventsIn]
  * @returns {Promise<Map<string, ResultStats>>}
  */
@@ -108,6 +108,10 @@ async function getStatsFromLogs(version, logPaths, getThreadId, trackEventsIn) {
 		const logs = JSON.parse(await readFile(logPath, 'utf8'));
 
 		let tid = getThreadId ? getThreadId(logs, logPath) : null;
+		if (tid == null) {
+			console.warn(`Could not find threadId for ${logPath}. Skipping...`);
+			continue;
+		}
 
 		/** @type {Array<{ id: string; start: number; end: number; }>} Determine what durations to track events under */
 		const parentLogs = [];
@@ -245,18 +249,10 @@ async function getStatsFromLogs(version, logPaths, getThreadId, trackEventsIn) {
 /**
  * @param {import('./tracing').TraceEvent[]} logs
  * @param {string} logFilePath
- * @returns {number}
+ * @returns {number | null}
  */
 function getDurationThread(logs, logFilePath) {
-	let log = logs.find(isDurationLog);
-
-	if (log == null) {
-		throw new Error(
-			`Could not find blink.user_timing log for "run-final" or "duration" in ${logFilePath}.`
-		);
-	} else {
-		return log.tid;
-	}
+	return logs.find(isDurationLog)?.tid ?? null;
 }
 
 /**
@@ -273,7 +269,8 @@ function isDurationLog(log) {
 	);
 }
 
-export async function analyze() {
+/** @param {string} requestedBench */
+export async function analyze(requestedBench) {
 	// const frameworkNames = await readdir(p('logs'));
 	const frameworkNames = frameworks.map(f => f.label);
 	const listAtEnd = [
@@ -296,6 +293,16 @@ export async function analyze() {
 	if (benchmarkNames.length == 0) {
 		console.log(`No benchmarks or results found in "${baseTraceLogDir()}".`);
 		return;
+	} else if (requestedBench) {
+		if (benchmarkNames.includes(requestedBench)) {
+			selectedBench = requestedBench;
+		} else {
+			console.log(
+				`Could not find benchmark "${requestedBench}". Available benchmarks:`
+			);
+			console.log(benchmarkNames);
+			return;
+		}
 	} else if (benchmarkNames.length == 1) {
 		selectedBench = benchmarkNames[0];
 	} else {

benches/scripts/index.js

@@ -99,12 +99,17 @@ prog
 	)
 	.action(runDeopts);
 
+// Test
+// - (no args)
+// - 02_replace1k
 prog
-	.command('analyze')
+	.command('analyze [benchmark]')
 	.describe(
 		'Analyze the trace logs created by running benchmarks with the --trace flag'
 	)
 	.example('analyze')
+	.example('analyze 02_replace1k')
+	.example('analyze many_updates')
 	.action(analyze);
 
 prog.parse(process.argv);