refactor!: remove *Concurrently methods (#135)

jerome-benoit · web-flow · commit 1297a9b40fd8 · 2024-10-18T11:18:20.000+02:00
Signed-off-by: Jérôme Benoit &lt;jerome.benoit@piment-noir.org&gt;
diff --git a/README.md b/README.md
@@ -131,9 +131,7 @@ export type Hook = (task: Task, mode: 'warmup' | 'run') => void | Promise<void>;
 ```
 
 - `async run()`: run the added tasks that were registered using the `add` method
-- `async runConcurrently(threshold: number = Infinity, mode: "bench" | "task" = "bench")`: similar to the `run` method but runs concurrently rather than sequentially. See the [Concurrency](#Concurrency) section.
 - `async warmup()`: warmup the benchmark tasks
-- `async warmupConcurrently(threshold: number = Infinity, mode: "bench" | "task" = "bench")`: warmup the benchmark tasks concurrently
 - `reset()`: reset each task and remove its result
 - `add(name: string, fn: Fn, opts?: FnOpts)`: add a benchmark task to the task map
   - `Fn`: `() => any | Promise<any>`
@@ -367,15 +365,10 @@ It may make your benchmarks slower, check #42.
 - When `mode` is set to 'bench', different tasks within the bench run concurrently. Concurrent cycles.
 
 ```ts
-// options way (recommended)
-bench.threshold = 10; // The maximum number of concurrent tasks to run. Defaults to Infinity.
+bench.threshold = 10; // The maximum number of concurrent tasks to run. Defaults to Number.POSITIVE_INFINITY.
 bench.concurrency = 'task'; // The concurrency mode to determine how tasks are run.
-// await bench.warmup();
+await bench.warmup();
 await bench.run();
-
-// standalone method way
-// await bench.warmupConcurrently(10, 'task');
-await bench.runConcurrently(10, 'task'); // with runConcurrently, mode is set to 'bench' by default
 ```
 
 ## Prior art
diff --git a/src/bench.ts b/src/bench.ts
@@ -30,12 +30,12 @@ export default class Bench extends EventTarget {
    *
    * - When `mode` is set to `null` (default), concurrency is disabled.
    * - When `mode` is set to 'task', each task's iterations (calls of a task function) run concurrently.
-   * - When `mode` is set to 'bench', different tasks within the bench run concurrently.
+   * - When `mode` is set to 'bench', different tasks within the bench run concurrently. Concurrent cycles.
    */
   concurrency: 'task' | 'bench' | null = null;
 
   /**
-   * The maximum number of concurrent tasks to run. Defaults to Infinity.
+   * The maximum number of concurrent tasks to run @default Number.POSITIVE_INFINITY
    */
   threshold = Number.POSITIVE_INFINITY;
 
@@ -88,105 +88,56 @@ export default class Bench extends EventTarget {
   }
 
   /**
-   * run the added tasks that were registered using the
-   * {@link add} method.
+   * run the added tasks that were registered using the {@link add} method.
    * Note: This method does not do any warmup. Call {@link warmup} for that.
    */
   async run(): Promise<Task[]> {
-    if (this.concurrency === 'bench') {
-      // TODO: in the next major, we should remove runConcurrently
-      return this.runConcurrently(this.threshold, this.concurrency);
-    }
+    let values: Task[] = [];
     this.dispatchEvent(createBenchEvent('start'));
-    const values: Task[] = [];
-    for (const task of [...this._tasks.values()]) {
-      values.push(await this.runTask(task));
+    if (this.concurrency === 'bench') {
+      const limit = pLimit(this.threshold);
+      const promises: Promise<Task>[] = [];
+      for (const task of this._tasks.values()) {
+        promises.push(limit(() => this.runTask(task)));
+      }
+      values = await Promise.all(promises);
+    } else {
+      for (const task of this._tasks.values()) {
+        values.push(await this.runTask(task));
+      }
     }
     this.dispatchEvent(createBenchEvent('complete'));
     return values;
   }
 
-  /**
-   * See Bench.{@link concurrency}
-   */
-  async runConcurrently(
-    threshold = Number.POSITIVE_INFINITY,
-    mode: NonNullable<Bench['concurrency']> = 'bench',
-  ): Promise<Task[]> {
-    this.threshold = threshold;
-    this.concurrency = mode;
-
-    if (mode === 'task') {
-      return this.run();
-    }
-
-    this.dispatchEvent(createBenchEvent('start'));
-
-    const limit = pLimit(threshold);
-
-    const promises: Promise<Task>[] = [];
-    for (const task of [...this._tasks.values()]) {
-      promises.push(limit(() => this.runTask(task)));
-    }
-
-    const values = await Promise.all(promises);
-
-    this.dispatchEvent(createBenchEvent('complete'));
-
-    return values;
-  }
-
   /**
    * warmup the benchmark tasks.
    * This is not run by default by the {@link run} method.
    */
   async warmup(): Promise<void> {
-    if (this.concurrency === 'bench') {
-      // TODO: in the next major, we should remove *Concurrently methods
-      await this.warmupConcurrently(this.threshold, this.concurrency);
-      return;
-    }
     this.dispatchEvent(createBenchEvent('warmup'));
-    for (const [, task] of this._tasks) {
-      await task.warmup();
-    }
-  }
-
-  /**
-   * warmup the benchmark tasks concurrently.
-   * This is not run by default by the {@link runConcurrently} method.
-   */
-  async warmupConcurrently(
-    threshold = Number.POSITIVE_INFINITY,
-    mode: NonNullable<Bench['concurrency']> = 'bench',
-  ): Promise<void> {
-    this.threshold = threshold;
-    this.concurrency = mode;
-
-    if (mode === 'task') {
-      await this.warmup();
-      return;
-    }
-
-    this.dispatchEvent(createBenchEvent('warmup'));
-    const limit = pLimit(threshold);
-    const promises: Promise<void>[] = [];
-
-    for (const [, task] of this._tasks) {
-      promises.push(limit(() => task.warmup()));
+    if (this.concurrency === 'bench') {
+      const limit = pLimit(this.threshold);
+      const promises: Promise<void>[] = [];
+      for (const task of this._tasks.values()) {
+        promises.push(limit(() => task.warmup()));
+      }
+      await Promise.all(promises);
+    } else {
+      for (const task of this._tasks.values()) {
+        await task.warmup();
+      }
     }
-
-    await Promise.all(promises);
   }
 
   /**
    * reset each task and remove its result
    */
   reset() {
     this.dispatchEvent(createBenchEvent('reset'));
-    this._tasks.forEach((task) => {
+    for (const task of this._tasks.values()) {
       task.reset();
-    });
+    }
   }
 
   /**
diff --git a/src/task.ts b/src/task.ts
@@ -39,8 +39,7 @@ export default class Task extends EventTarget {
   fn: Fn;
 
   /*
-   * The number of times the task
-   * function has been executed
+   * The number of times the task function has been executed
    */
   runs = 0;
 
@@ -60,15 +59,13 @@ export default class Task extends EventTarget {
     this.name = name;
     this.fn = fn;
     this.opts = opts;
-    // TODO: support signals in Tasks
+    // TODO: support signal in Tasks
   }
 
   private async loop(
     time: number,
     iterations: number,
   ): Promise<{ error?: unknown; samples?: number[] }> {
-    const concurrent = this.bench.concurrency === 'task';
-    const { threshold } = this.bench;
     let totalTime = 0; // ms
     const samples: number[] = [];
     if (this.opts.beforeAll != null) {
@@ -105,22 +102,21 @@ export default class Task extends EventTarget {
         await this.opts.afterEach.call(this);
       }
     };
-
-    const limit = pLimit(threshold);
     try {
+      const limit = pLimit(this.bench.threshold); // only for task level concurrency
       const promises: Promise<void>[] = []; // only for task level concurrency
       while (
         (totalTime < time
           || samples.length + limit.activeCount + limit.pendingCount < iterations)
         && !this.bench.signal?.aborted
       ) {
-        if (concurrent) {
+        if (this.bench.concurrency === 'task') {
           promises.push(limit(executeTask));
         } else {
           await executeTask();
         }
       }
-      if (promises.length) {
+      if (promises.length > 0) {
         await Promise.all(promises);
       }
     } catch (error) {
@@ -138,7 +134,7 @@ export default class Task extends EventTarget {
   }
 
   /**
-   * run the current task and write the results in `Task.result` object
+   * run the current task and write the results in `Task.result` object property
    */
   async run() {
     if (this.result?.error) {
@@ -272,8 +268,7 @@ export default class Task extends EventTarget {
   }
 
   /**
-   * reset the task to make the `Task.runs` a zero-value and remove the `Task.result`
-   * object
+   * reset the task to make the `Task.runs` a zero-value and remove the `Task.result` object property
    */
   reset() {
     this.dispatchEvent(createBenchEvent('reset', this));
diff --git a/test/index.test.ts b/test/index.test.ts
@@ -154,7 +154,7 @@ test('events order', async () => {
   expect(abortTask.result).toBeUndefined();
 }, 10000);
 
-test('events order 2', async () => {
+test('events order at task completion', async () => {
   const bench = new Bench({
     warmupIterations: 0,
     warmupTime: 0,
@@ -174,17 +174,19 @@ test('events order 2', async () => {
   const barTask = bench.getTask('bar')!;
   fooTask.addEventListener('complete', () => {
     events.push('foo-complete');
-    expect(events).not.toContain('bar-complete');
+    expect(events).toStrictEqual(['foo-complete']);
   });
 
   barTask.addEventListener('complete', () => {
     events.push('bar-complete');
-    expect(events).toContain('foo-complete');
+    expect(events).toStrictEqual(['foo-complete', 'bar-complete']);
   });
 
-  await bench.run();
+  const tasks = await bench.run();
 
-  await new Promise((resolve) => setTimeout(resolve, 150));
+  expect(tasks.length).toBe(2);
+  expect(tasks[0]?.name).toBe('foo');
+  expect(tasks[1]?.name).toBe('bar');
 });
 
 test('error event', async () => {
diff --git a/test/sequential.test.ts b/test/sequential.test.ts
