From f9c70abc8f972b5b6f2eead0bfec37115b37f0c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 2 Jan 2025 07:24:01 -0800 Subject: [PATCH] Run benchmarks in test mode when not specifying verification functions in CI Summary: Changelog: [internal] Modifies the execution of benchmarks in CI to run benchmarks in test mode when they don't define a `verify` method. If a benchmark uses `verify`, the test is meant to make sure that the benchmark doesn't regress in CI. If it doesn't, then running the benchmark on CI doesn't provide much value. In that case, we run a single iteration of each test case just to make sure things don't break over time. Differential Revision: D67637754 --- .../runner/entrypoint-template.js | 7 +++ packages/react-native-fantom/runner/runner.js | 2 + packages/react-native-fantom/runner/utils.js | 10 ++++ packages/react-native-fantom/src/Benchmark.js | 56 ++++++++++++++----- packages/react-native-fantom/src/index.js | 16 ++++++ 5 files changed, 78 insertions(+), 13 deletions(-) diff --git a/packages/react-native-fantom/runner/entrypoint-template.js b/packages/react-native-fantom/runner/entrypoint-template.js index c52c66209e0e8a..dc99d9f0551d84 100644 --- a/packages/react-native-fantom/runner/entrypoint-template.js +++ b/packages/react-native-fantom/runner/entrypoint-template.js @@ -18,12 +18,14 @@ module.exports = function entrypointTemplate({ featureFlagsModulePath, featureFlags, snapshotConfig, + isRunningFromCI, }: { testPath: string, setupModulePath: string, featureFlagsModulePath: string, featureFlags: FantomTestConfigJsOnlyFeatureFlags, snapshotConfig: SnapshotConfig, + isRunningFromCI: boolean, }): string { return `/** * Copyright (c) Meta Platforms, Inc. and affiliates. @@ -38,6 +40,7 @@ module.exports = function entrypointTemplate({ */ import {registerTest} from '${setupModulePath}'; +import {setConstants} from '@react-native/fantom'; ${ Object.keys(featureFlags).length > 0 ? `import * as ReactNativeFeatureFlags from '${featureFlagsModulePath}'; @@ -50,6 +53,10 @@ ${Object.entries(featureFlags) : '' } +setConstants({ + isRunningFromCI: ${String(isRunningFromCI)}, +}); + registerTest(() => require('${testPath}'), ${JSON.stringify(snapshotConfig)}); `; }; diff --git a/packages/react-native-fantom/runner/runner.js b/packages/react-native-fantom/runner/runner.js index 776ee43957b301..3c8050548ad4e8 100644 --- a/packages/react-native-fantom/runner/runner.js +++ b/packages/react-native-fantom/runner/runner.js @@ -22,6 +22,7 @@ import { getBuckModesForPlatform, getDebugInfoFromCommandResult, getShortHash, + isRunningFromCI, runBuck2, symbolicateStackTrace, } from './utils'; @@ -145,6 +146,7 @@ module.exports = async function runTest( updateSnapshot: snapshotState._updateSnapshot, data: getInitialSnapshotData(snapshotState), }, + isRunningFromCI: isRunningFromCI(), }); const entrypointPath = path.join( diff --git a/packages/react-native-fantom/runner/utils.js b/packages/react-native-fantom/runner/utils.js index d5aa6deeac7ab3..ef0996a39e71f5 100644 --- a/packages/react-native-fantom/runner/utils.js +++ b/packages/react-native-fantom/runner/utils.js @@ -44,6 +44,16 @@ export function getBuckModesForPlatform( return ['@//xplat/mode/react-force-cxx-platform', osPlatform]; } +function isEmpty(value: ?string): boolean { + return value == null || value === ''; +} + +export function isRunningFromCI(): boolean { + return ( + !isEmpty(process.env.SANDCASTLE) || !isEmpty(process.env.GITHUB_ACTIONS) + ); +} + type SpawnResultWithOriginalCommand = { ...ReturnType, originalCommand: string, diff --git a/packages/react-native-fantom/src/Benchmark.js b/packages/react-native-fantom/src/Benchmark.js index 036c94efeadff0..7c2c22d5b6c7de 100644 --- a/packages/react-native-fantom/src/Benchmark.js +++ b/packages/react-native-fantom/src/Benchmark.js @@ -8,6 +8,7 @@ * @format */ +import {getConstants} from './index'; import nullthrows from 'nullthrows'; import NativeCPUTime from 'react-native/src/private/specs/modules/NativeCPUTime'; import { @@ -36,31 +37,60 @@ export function suite( suiteName: string, suiteOptions?: ?SuiteOptions, ): SuiteAPI { - const {disableOptimizedBuildCheck, ...benchOptions} = suiteOptions ?? {}; - - const bench = new Bench({ - ...benchOptions, - name: suiteName, - throws: true, - now: () => NativeCPUTime.getCPUTimeNanos() / 1000000, - }); - + const tasks: Array<{ + name: string, + fn: () => void, + options: FnOptions | void, + }> = []; const verifyFns = []; global.it(suiteName, () => { - if (bench.tasks.length === 0) { + if (tasks.length === 0) { throw new Error('No benchmark tests defined'); } + const {isRunningFromCI} = getConstants(); + + // If we're running from CI and there's no verification function, there's + // no point in running the benchmark. + // We still run a single iteration of each test just to make sure that the + // logic in the benchmark doesn't break. + const isTestOnly = isRunningFromCI && verifyFns.length === 0; + + const overriddenOptions: BenchOptions = isTestOnly + ? { + warmupIterations: 1, + warmupTime: 0, + iterations: 1, + time: 0, + } + : {}; + + const {disableOptimizedBuildCheck, ...benchOptions} = suiteOptions ?? {}; + + const bench = new Bench({ + ...benchOptions, + ...overriddenOptions, + name: suiteName, + throws: true, + now: () => NativeCPUTime.getCPUTimeNanos() / 1000000, + }); + + for (const task of tasks) { + bench.add(task.name, task.fn, task.options); + } + bench.runSync(); - printBenchmarkResults(bench); + if (!isTestOnly) { + printBenchmarkResults(bench); + } for (const verify of verifyFns) { verify(bench.results); } - if (!NativeCPUTime.hasAccurateCPUTimeNanos()) { + if (!isTestOnly && !NativeCPUTime.hasAccurateCPUTimeNanos()) { throw new Error( '`NativeCPUTime` module does not provide accurate CPU time information in this environment. Please run the benchmarks in an environment where it does.', ); @@ -73,7 +103,7 @@ export function suite( const suiteAPI = { add(name: string, fn: () => void, options?: FnOptions): SuiteAPI { - bench.add(name, fn, options); + tasks.push({name, fn, options}); return suiteAPI; }, verify(fn: (results: SuiteResults) => void): SuiteAPI { diff --git a/packages/react-native-fantom/src/index.js b/packages/react-native-fantom/src/index.js index b3c0d513f0dbf2..195cc19fcca193 100644 --- a/packages/react-native-fantom/src/index.js +++ b/packages/react-native-fantom/src/index.js @@ -115,6 +115,22 @@ export function createRoot(): Root { export const benchmark = Benchmark; +type FantomConstants = $ReadOnly<{ + isRunningFromCI: boolean, +}>; + +let constants: FantomConstants = { + isRunningFromCI: false, +}; + +export function getConstants(): FantomConstants { + return constants; +} + +export function setConstants(newConstants: FantomConstants): void { + constants = newConstants; +} + /** * Quick and dirty polyfills required by tinybench. */