Skip to content

Commit

Permalink
fix: cleanup options when suite finishes (#24)
Browse files Browse the repository at this point in the history
* Clean up options at task level

* Add debug package

* Add debug logging

* Add more debug logs
nazarhussain authored Jan 22, 2025
1 parent dfe1220 commit a2a13e7
Showing 8 changed files with 78 additions and 28 deletions.
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
@@ -40,6 +40,7 @@
"@eslint/js": "^9.15.0",
"@types/node": "^22.10.2",
"@types/yargs": "^17.0.33",
"@types/debug": "^4.1.12",
"dotenv": "^10.0.0",
"eslint": "^9.15.0",
"eslint-config-prettier": "^9.1.0",
@@ -62,6 +63,7 @@
"aws-sdk": "^2.932.0",
"csv-parse": "^5.6.0",
"csv-stringify": "^6.5.2",
"debug": "^4.4.0",
"glob": "^10.4.5",
"log-symbols": "^7.0.0",
"yaml": "^2.7.0",
22 changes: 20 additions & 2 deletions src/benchmark/benchmarkFn.ts
Original file line number Diff line number Diff line change
@@ -65,8 +65,21 @@ export const bench: BenchApi = createBenchmarkFunction(function <T, T2>(
},
});

const {id: _, ...optionsWithoutId} = opts;
setFn(task, handler);
store.setOptions(task, opts);
store.setOptions(task, optionsWithoutId);

task.onFinished = [
() => {
store.removeOptions(task);
},
() => {
// Clear up the assigned handler to clean the memory
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-expect-error
setFn(task, null);
},
];
});

function createBenchmarkFunction(
@@ -123,7 +136,12 @@ function coerceToOptsObj<T, T2>(
* ```
*/
export function setBenchOpts(opts: BenchmarkOpts): void {
store.setOptions(getCurrentSuite(), opts);
const suite = getCurrentSuite();
store.setOptions(suite, opts);

suite.on("afterAll", () => {
store.removeOptions(suite);
});
}

export const setBenchmarkOptions = setBenchOpts;
13 changes: 11 additions & 2 deletions src/benchmark/globalState.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import Debug from "debug";
import type {Suite, SuiteCollector, Task} from "@vitest/runner";
import {BenchmarkResult, BenchmarkOpts, BenchmarkResults} from "../types.js";

const debug = Debug("@chainsafe/benchmark/state");

/**t
* Map of results by root suite.
*/
@@ -14,13 +17,14 @@ let globalOpts: BenchmarkOpts | undefined;
/**
* Map to persist options set in describe blocks
*/
const optsMap = new WeakMap<object, BenchmarkOpts>();
const optsMap = new Map<object, BenchmarkOpts>();

export const store = {
getResult(id: string): BenchmarkResult | undefined {
return results.get(id);
},
setResult(id: string, result: BenchmarkResult): void {
debug("setting result for %o", id);
results.set(id, result);
},
getAllResults(): BenchmarkResults {
@@ -30,12 +34,17 @@ export const store = {
return optsMap.get(suite);
},
setOptions(suite: Task | Suite | SuiteCollector, opts: BenchmarkOpts): void {
if (Object.keys(opts).length === 0) return;

debug("setting options for %o with name %o %O", suite.type, suite.name, opts);
optsMap.set(suite, opts);
},
removeOptions(suite: Task | Suite): void {
removeOptions(suite: Task | Suite | SuiteCollector): void {
debug("removing options for %o with name %o", suite.type, suite.name);
optsMap.delete(suite);
},
setGlobalOptions(opts: Partial<BenchmarkOpts>): void {
debug("setting global options %O", opts);
globalOpts = opts;
},
getGlobalOptions(): BenchmarkOpts | undefined {
25 changes: 16 additions & 9 deletions src/benchmark/runner.ts
Original file line number Diff line number Diff line change
@@ -6,13 +6,15 @@ import {
VitestRunner,
VitestRunnerConfig,
VitestRunnerImportSource,
setFn,
} from "@vitest/runner";
import path from "node:path";
import Debug from "debug";
import {Benchmark, BenchmarkOpts, BenchmarkResults} from "../types.js";
import {BenchmarkReporter} from "./reporter.js";
import {store} from "./globalState.js";

const debug = Debug("@chainsafe/benchmark/runner");

export class BenchmarkRunner implements VitestRunner {
readonly triggerGC: boolean;
readonly config: VitestRunnerConfig;
@@ -43,24 +45,26 @@ export class BenchmarkRunner implements VitestRunner {

onAfterRunSuite(suite: Suite): void {
this.reporter.onSuiteFinished(suite);
store.removeOptions(suite);
}

onBeforeRunTask(task: Task): void {
this.reporter.onTestStarted(task);
}

async onAfterRunTask(task: Task): Promise<void> {
async onTaskFinished(task: Task): Promise<void> {
this.reporter.onTestFinished(task);
store.removeOptions(task);

if (task.type === "test" || task.type === "custom") {
// Clear up the assigned handler to clean the memory
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-expect-error
setFn(task, null);
// To help maintain consistent memory usage patterns
// we trigger garbage collection manually
if (this.triggerGC && global.gc) {
global.gc();
// Make sure the syn operation is off the event loop
await new Promise((resolve) => setTimeout(resolve, 0));
}
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
async onAfterRunTask(task: Task): Promise<void> {
// To help maintain consistent memory usage patterns
// we trigger garbage collection manually
if (this.triggerGC && global.gc) {
@@ -86,12 +90,15 @@ export class BenchmarkRunner implements VitestRunner {
async process(files: string[]): Promise<BenchmarkResults> {
store.setGlobalOptions(this.benchmarkOpts);

debug("starting tests %O", files);
const res = await startTests(files, this);

const passed = res.filter((r) => r.result?.state == "pass");
const skipped = res.filter((r) => r.result?.state == "skip");
const failed = res.filter((r) => r.result?.state == "fail");

debug("finished tests. passed: %i, skipped: %i, failed: %i", passed.length, skipped.length, failed.length);

if (failed.length > 0) {
throw failed[0].result?.errors;
}
5 changes: 5 additions & 0 deletions src/cli/cli.ts
Original file line number Diff line number Diff line change
@@ -2,9 +2,12 @@
import yargs from "yargs";
import fs from "node:fs";
import path from "node:path";
import Debug from "debug";
import {parse as parseYaml} from "yaml";
import {hideBin} from "yargs/helpers";

const debug = Debug("@chainsafe/benchmark/cli");

import {benchmarkOptions, CLIOptions, fileCollectionOptions, storageOptions} from "./options.js";
import {run} from "./run.js";
import {compare} from "./compare.js";
@@ -20,6 +23,7 @@ void yargs(hideBin(process.argv))
},
handler: async (argv) => {
const cliOpts = {...argv} as unknown as CLIOptions & {spec: string[]};
debug("Executing command run with %O", cliOpts);

await run(cliOpts);
},
@@ -33,6 +37,7 @@ void yargs(hideBin(process.argv))
},
handler: async (argv) => {
const cliOpts = {...argv} as unknown as {dirs: string[]};
debug("Executing command compare with %O", cliOpts);

await compare(cliOpts);
},
6 changes: 6 additions & 0 deletions src/cli/run.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as github from "@actions/github";
import Debug from "debug";
import {getHistoryProvider} from "../history/index.js";
import {resolveShouldPersist} from "../history/shouldPersist.js";
import {validateBenchmark} from "../history/schema.js";
@@ -22,6 +23,8 @@ import {HistoryProviderType} from "../history/provider.js";
import {performanceReportComment} from "../github/comments/performanceReportComment.js";
import {GithubCommentTag} from "../github/octokit.js";

const debug = Debug("@chainsafe/benchmark/cli");

export async function run(opts_: FileCollectionOptions & StorageOptions & BenchmarkOpts): Promise<void> {
const opts = Object.assign({}, optionsDefault, opts_);

@@ -70,6 +73,8 @@ export async function run(opts_: FileCollectionOptions & StorageOptions & Benchm
// Persist new benchmark data
const currentBranch = await getCurrentBranch();
const shouldPersist = await resolveShouldPersist(opts, currentBranch);

debug("detecting to persist results. found: %o", shouldPersist);
if (shouldPersist === true) {
const branch =
currentCommit.branch ??
@@ -86,6 +91,7 @@ export async function run(opts_: FileCollectionOptions & StorageOptions & Benchm

const resultsComp = computePerformanceReport(currBench, prevBench, opts.threshold);

debug("detecting to post comment. skipPostComment: %o, isGaRun: %o", !opts.skipPostComment, isGaRun());
if (!opts.skipPostComment && isGaRun()) {
await postGaComment({
commentBody: performanceReportComment(resultsComp),
19 changes: 5 additions & 14 deletions test/perf/iteration.test.ts
Original file line number Diff line number Diff line change
@@ -35,8 +35,8 @@ describe("Array iteration", () => {
bench({
id: "sum array with reduce beforeEach",
beforeEach: () => Array.from({length: 1e4}, (_, i) => i),
fn: () => {
arr.reduce((total, curr) => total + curr, 0);
fn: (arrayFromBeforeEach) => {
arrayFromBeforeEach.reduce((total, curr) => total + curr, 0);

// Uncomment below to cause a guaranteed performance regression
// arr.reduce((total, curr) => total + curr, 0);
@@ -47,26 +47,17 @@ describe("Array iteration", () => {
bench({
id: "sum array with reduce before beforeEach",
before: () => Array.from({length: 1e4}, (_, i) => i),
beforeEach: (arr) => arr.slice(0),
fn: () => {
arr.reduce((total, curr) => total + curr, 0);
beforeEach: (arrFromBefore) => arrFromBefore.slice(0),
fn: (arrayFromBeforeEach) => {
arrayFromBeforeEach.reduce((total, curr) => total + curr, 0);

// Uncomment below to cause a guaranteed performance regression
// arr.reduce((total, curr) => total + curr, 0);
// arr.reduce((total, curr) => total + curr, 0);
},
});

bench.skip("sum array with reduce", () => {
arr.reduce((total, curr) => total + curr, 0);
});

// bench.only("sum array with reduce", () => {
// arr.reduce((total, curr) => total + curr, 0);
// });

// Reporter options

bench({
id: "sum array with reduce high threshold",
threshold: 5,
14 changes: 13 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
@@ -768,6 +768,13 @@
resolved "https://registry.yarnpkg.com/@tsconfig/node16/-/node16-1.0.4.tgz#0b92dcc0cc1c81f6f306a381f28e31b1a56536e9"
integrity sha512-vxhUy4J8lyeyinH7Azl1pdd43GJhZH/tP2weN8TntQblOY+A0XbT8DJk1/oCPuOOyg/Ja757rG0CgHcWC8OfMA==

"@types/debug@^4.1.12":
version "4.1.12"
resolved "https://registry.yarnpkg.com/@types/debug/-/debug-4.1.12.tgz#a155f21690871953410df4b6b6f53187f0500917"
integrity sha512-vIChWdVG3LG1SMxEvI/AK+FWJthlrqlTu7fbrlywTkkaONwk/UAGaULXRlf8vkzFBLVm0zkMdCquhL5aOjhXPQ==
dependencies:
"@types/ms" "*"

"@types/[email protected]", "@types/estree@^1.0.0", "@types/estree@^1.0.6":
version "1.0.6"
resolved "https://registry.yarnpkg.com/@types/estree/-/estree-1.0.6.tgz#628effeeae2064a1b4e79f78e81d87b7e5fc7b50"
@@ -783,6 +790,11 @@
resolved "https://registry.yarnpkg.com/@types/json5/-/json5-0.0.29.tgz#ee28707ae94e11d2b827bcbe5270bcea7f3e71ee"
integrity sha1-7ihweulOEdK4J7y+UnC86n8+ce4=

"@types/ms@*":
version "2.1.0"
resolved "https://registry.yarnpkg.com/@types/ms/-/ms-2.1.0.tgz#052aa67a48eccc4309d7f0191b7e41434b90bb78"
integrity sha512-GsCCIZDE/p3i96vtEqx+7dBUGXrc7zeSK3wwPHIaRThS+9OhWIXRqzs4d6k1SVU8g91DrNRWxWUGhp5KXQb2VA==

"@types/node@^22.10.2":
version "22.10.2"
resolved "https://registry.yarnpkg.com/@types/node/-/node-22.10.2.tgz#a485426e6d1fdafc7b0d4c7b24e2c78182ddabb9"
@@ -1331,7 +1343,7 @@ data-view-byte-offset@^1.0.0:
es-errors "^1.3.0"
is-data-view "^1.0.1"

debug@4, debug@^4.3.7:
debug@4, debug@^4.3.7, debug@^4.4.0:
version "4.4.0"
resolved "https://registry.yarnpkg.com/debug/-/debug-4.4.0.tgz#2b3f2aea2ffeb776477460267377dc8710faba8a"
integrity sha512-6WTZ/IxCY/T6BALoZHaE4ctp9xm+Z5kY/pzYaCHRFeyVhojxlrm+46y68HA6hr0TcwEssoxNiDEUJQjfPZ/RYA==

0 comments on commit a2a13e7

Please sign in to comment.