feat(benchmark infra) Add beforeEachBatchAsync callback #23391
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,8 +95,7 @@ for (const type of Object.values(TestType)) { | |
| * Arguments to `benchmark` | ||
| * @public | ||
| */ | ||
| export type BenchmarkArguments = Titled & BenchmarkRunningOptions; | ||
|
|
||
| /** | ||
| * @public | ||
|
|
@@ -113,11 +112,9 @@ export type BenchmarkRunningOptions = | |
| | BenchmarkAsyncArguments | ||
| | CustomBenchmarkArguments; | ||
|
|
||
| export type BenchmarkRunningOptionsSync = BenchmarkSyncArguments; | ||
|
|
||
| export type BenchmarkRunningOptionsAsync = BenchmarkAsyncArguments; | ||
|
Comment on lines
+115
to
+117
There was a problem hiding this comment. Nit: these two feel redundant now, maybe we should remove them? There was a problem hiding this comment. Fine with me. @CraigMacomber ? |
||
|
|
||
| /** | ||
| * Object with a "title". | ||
|
|
@@ -134,13 +131,13 @@ export interface Titled { | |
| * Arguments to benchmark a synchronous function | ||
| * @public | ||
| */ | ||
| export interface BenchmarkSyncArguments extends BenchmarkSyncFunction, BenchmarkOptions, OnBatch {} | ||
|
|
||
| /** | ||
| * Arguments to benchmark a synchronous function | ||
| * @public | ||
| */ | ||
| export interface BenchmarkSyncFunction { | ||
| /** | ||
| * The (synchronous) function to benchmark. | ||
| */ | ||
|
|
@@ -151,13 +148,16 @@ export interface BenchmarkSyncFunction extends BenchmarkOptions { | |
| * Configuration for benchmarking an asynchronous function. | ||
| * @public | ||
| */ | ||
| export interface BenchmarkAsyncArguments | ||
| extends BenchmarkAsyncFunction, | ||
| BenchmarkOptions, | ||
| OnBatchAsync {} | ||
|
|
||
| /** | ||
| * An asynchronous function to benchmark. | ||
| * @public | ||
| */ | ||
| export interface BenchmarkAsyncFunction { | ||
| /** | ||
| * The asynchronous function to benchmark. The time measured includes all time spent until the returned promise is | ||
| * resolved. This includes the event loop or processing other events. For example, a test which calls `setTimeout` | ||
|
|
@@ -209,20 +209,57 @@ export interface BenchmarkTimingOptions { | |
| } | ||
|
|
||
| /** | ||
| * Synchronous operations that can be performed on a per-batch basis. | ||
| * | ||
| * @remarks | ||
| * If you need to perform asynchronous operations, use {@link BenchmarkAsyncFunction} and {@link OnBatchAsync}. | ||
| * | ||
| * @public | ||
| */ | ||
| export interface OnBatch { | ||
| /** | ||
| * Executes synchronously before the start of each batch. | ||
| * This has the same semantics as benchmarkjs's `onCycle`: | ||
| * https://benchmarkjs.com/docs/#options_onCycle | ||
| * | ||
| * @remarks | ||
| * Beware that batches run `benchmarkFn` more than once: a typical micro-benchmark might involve 10k | ||
| * iterations per batch. | ||
| */ | ||
| beforeEachBatch?: () => void; | ||
|
|
||
| /** | ||
| * Use {@link BenchmarkAsyncFunction} and {@link OnBatchAsync} if you need an async hook. | ||
| */ | ||
| beforeEachBatchAsync?: never; | ||
| } | ||
|
|
||
| /** | ||
| * Operations (synchronous or asynchronous) that can be performed on a per-batch basis. | ||
| * | ||
| * @remarks | ||
| * Not compatible with synchronous {@link BenchmarkSyncFunction}. | ||
| * | ||
| * @public | ||
| */ | ||
| export interface OnBatchAsync { | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. The key feature is to block synchronous benchmark with asynchronous beforeEachBatch. I tried a number of strategies, this was the best I came up with. If you can see a simpler way to keep the tests passing, I'm all for it. Or maybe that requirement I held to isn't worth it? Seems like it to me but open to discussion. There was a problem hiding this comment. I deprecated the one in the Async type because it's unnecessary, as you pointed out. Later someone could migrate usages to the new Async function and remove that one. |
||
| /** | ||
| * Executes synchronously before the start of each batch. | ||
| * | ||
| * @deprecated use {@link OnBatchAsync.beforeEachBatchAsync} instead | ||
| */ | ||
| beforeEachBatch?: () => void; | ||
|
|
||
| /** | ||
| * Executes before the start of each batch. | ||
| * This has similar semantics to benchmarkjs's `onCycle`, but is asynchronous: | ||
| * https://benchmarkjs.com/docs/#options_onCycle | ||
| * | ||
| * @remarks | ||
| * Beware that batches run `benchmarkFn` more than once: a typical micro-benchmark might involve 10k | ||
| * iterations per batch. | ||
| */ | ||
| beforeEachBatchAsync?: () => Promise<void>; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -234,7 +271,6 @@ export interface BenchmarkOptions | |
| extends MochaExclusiveOptions, | ||
| HookArguments, | ||
| BenchmarkTimingOptions, | ||
| BenchmarkDescription {} | ||
|
|
||
| /** | ||
|
|
@@ -386,30 +422,45 @@ export interface HookArguments { | |
| } | ||
|
|
||
| /** | ||
| * Type guard to distinguish between synchronous and asynchronous benchmark arguments. | ||
| * @param args - Either {@link BenchmarkSyncArguments} or {@link BenchmarkAsyncArguments} | ||
| * @returns true if the arguments are for an asynchronous benchmark, false if they are for a synchronous benchmark. | ||
| */ | ||
| function isAsync( | ||
| args: BenchmarkSyncArguments | BenchmarkAsyncArguments, | ||
| ): args is BenchmarkAsyncArguments { | ||
| const intersection = args as BenchmarkSyncArguments & BenchmarkAsyncArguments; | ||
| const isSync = intersection.benchmarkFn !== undefined; | ||
| const isAsync = intersection.benchmarkFnAsync !== undefined; | ||
| assert( | ||
| isSync !== isAsync, | ||
| "Exactly one of `benchmarkFn` and `benchmarkFnAsync` should be defined.", | ||
| ); | ||
| return isAsync; | ||
| } | ||
|
|
||
| /** | ||
| * Validates arguments to `benchmark`. | ||
| * @public | ||
| */ | ||
| export function validateBenchmarkArguments( | ||
| args: BenchmarkSyncArguments | BenchmarkAsyncArguments, | ||
| ): | ||
| | { isAsync: true; benchmarkFn: () => Promise<unknown>; beforeEachBatch?: () => Promise<void> } | ||
| | { isAsync: false; benchmarkFn: () => void; beforeEachBatch?: () => void } { | ||
| if (isAsync(args)) { | ||
| const beforeEachBatch = async (): Promise<void> => { | ||
| args.beforeEachBatch?.(); | ||
| await args.beforeEachBatchAsync?.(); | ||
| }; | ||
| return { isAsync: true, benchmarkFn: args.benchmarkFnAsync, beforeEachBatch }; | ||
| } | ||
|
|
||
| return { isAsync: false, benchmarkFn: args.benchmarkFn, beforeEachBatch: args.beforeEachBatch }; | ||
| } | ||
|
|
||
| /** | ||
| * type guard for {@link BenchmarkRunningOptions} to determine if the provided arguments are for a custom benchmark. | ||
| * @public | ||
| */ | ||
| export function benchmarkArgumentsIsCustom( | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This change seems unnecessary complex. While we have to make the actual function being timed clearly sync or async to avoid the over head of checking which each iteration, I don't see an issue with making
beforeEachBatch?: () => void;just typed asbeforeEachBatch?: () => void | promise<unknown>;then just awaiting what ever it returns unconditionally.That should keep the API identical, except that async beforeEachBatch will work. This is similar to how mocha deals with hooks: same API for sync and async.
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.
That's how I started. I was bothered by the lack of compiler error when passing args with synchronous benchmarkFn by async beforeEachBatch
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.
Looking at the implementation, I guess that would be an issue with how a few things are currently factored, like
runBenchmarkSync, so some of this refactoring might be needed to support async beforeEachBatch.