From 9cd39dd47830f0923cd3ebf53b709bf7fb07e788 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Wed, 5 Apr 2023 12:40:40 +0200 Subject: [PATCH] perf: remove unneeded class indirection for dynamic pool in worker selection code MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Signed-off-by: Jérôme Benoit --- CHANGELOG.md | 4 ++ .../worker-selection/{lru.js => less.js} | 0 src/pools/cluster/dynamic.ts | 2 +- src/pools/pool-internal.ts | 4 +- .../abstract-worker-choice-strategy.ts | 2 +- .../dynamic-pool-worker-choice-strategy.ts | 67 ------------------- .../selection-strategies-types.ts | 2 +- .../worker-choice-strategy-context.ts | 37 ++++------ src/pools/thread/dynamic.ts | 2 +- .../selection-strategies.test.js | 41 +++++------- .../worker-choice-strategy-context.test.js | 28 ++------ 11 files changed, 45 insertions(+), 144 deletions(-) rename benchmarks/worker-selection/{lru.js => less.js} (100%) delete mode 100644 src/pools/selection-strategies/dynamic-pool-worker-choice-strategy.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f1b026dc..bc047e43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Optimize worker choice strategy for dynamic pool. + ### Fixed - Ensure dynamic pool does not alter worker choice strategy expected behavior. diff --git a/benchmarks/worker-selection/lru.js b/benchmarks/worker-selection/less.js similarity index 100% rename from benchmarks/worker-selection/lru.js rename to benchmarks/worker-selection/less.js diff --git a/src/pools/cluster/dynamic.ts b/src/pools/cluster/dynamic.ts index c4fc788c..79f684cb 100644 --- a/src/pools/cluster/dynamic.ts +++ b/src/pools/cluster/dynamic.ts @@ -6,7 +6,7 @@ import { FixedClusterPool } from './fixed' * A cluster pool with a dynamic number of workers, but a guaranteed minimum number of workers. * * This cluster pool creates new workers when the others are busy, up to the maximum number of workers. - * When the maximum number of workers is reached, an event is emitted. If you want to listen to this event, use the pool's `emitter`. + * When the maximum number of workers is reached and workers are busy, an event is emitted. If you want to listen to this event, use the pool's `emitter`. * * @typeParam Data - Type of data sent to the worker. This can only be serializable data. * @typeParam Response - Type of response of execution. This can only be serializable data. diff --git a/src/pools/pool-internal.ts b/src/pools/pool-internal.ts index 806107ec..2d92f2f0 100644 --- a/src/pools/pool-internal.ts +++ b/src/pools/pool-internal.ts @@ -34,8 +34,8 @@ export interface WorkerType { * Internal contract definition for a poolifier pool. * * @typeParam Worker - Type of worker which manages this pool. - * @typeParam Data - Type of data sent to the worker. - * @typeParam Response - Type of response of execution. + * @typeParam Data - Type of data sent to the worker. This can only be serializable data. + * @typeParam Response - Type of response of execution. This can only be serializable data. */ export interface IPoolInternal< Worker extends IPoolWorker, diff --git a/src/pools/selection-strategies/abstract-worker-choice-strategy.ts b/src/pools/selection-strategies/abstract-worker-choice-strategy.ts index 3a5d6e04..aa370fe4 100644 --- a/src/pools/selection-strategies/abstract-worker-choice-strategy.ts +++ b/src/pools/selection-strategies/abstract-worker-choice-strategy.ts @@ -7,7 +7,7 @@ import type { } from './selection-strategies-types' /** - * Abstract worker choice strategy class. + * Worker choice strategy abstract base class. * * @typeParam Worker - Type of worker which manages the strategy. * @typeParam Data - Type of data sent to the worker. This can only be serializable data. diff --git a/src/pools/selection-strategies/dynamic-pool-worker-choice-strategy.ts b/src/pools/selection-strategies/dynamic-pool-worker-choice-strategy.ts deleted file mode 100644 index 930675f2..00000000 --- a/src/pools/selection-strategies/dynamic-pool-worker-choice-strategy.ts +++ /dev/null @@ -1,67 +0,0 @@ -import type { IPoolInternal } from '../pool-internal' -import type { IPoolWorker } from '../pool-worker' -import { AbstractWorkerChoiceStrategy } from './abstract-worker-choice-strategy' -import type { - IWorkerChoiceStrategy, - WorkerChoiceStrategy -} from './selection-strategies-types' -import { WorkerChoiceStrategies } from './selection-strategies-types' -import { getWorkerChoiceStrategy } from './selection-strategies-utils' - -/** - * Selects the next worker for dynamic pool. - * - * @typeParam Worker - Type of worker which manages the strategy. - * @typeParam Data - Type of data sent to the worker. This can only be serializable data. - * @typeParam Response - Type of response of execution. This can only be serializable data. - */ -export class DynamicPoolWorkerChoiceStrategy< - Worker extends IPoolWorker, - Data, - Response - > - extends AbstractWorkerChoiceStrategy - implements IWorkerChoiceStrategy { - private readonly workerChoiceStrategy: IWorkerChoiceStrategy - - /** - * Constructs a worker choice strategy for dynamic pool. - * - * @param pool - The pool instance. - * @param createWorkerCallback - The worker creation callback for dynamic pool. - * @param workerChoiceStrategy - The worker choice strategy when the pool is busy. - */ - public constructor ( - pool: IPoolInternal, - private readonly createWorkerCallback: () => number, - workerChoiceStrategy: WorkerChoiceStrategy = WorkerChoiceStrategies.ROUND_ROBIN - ) { - super(pool) - this.workerChoiceStrategy = getWorkerChoiceStrategy( - this.pool, - workerChoiceStrategy - ) - this.requiredStatistics = this.workerChoiceStrategy.requiredStatistics - } - - /** {@inheritDoc} */ - public reset (): boolean { - return this.workerChoiceStrategy.reset() - } - - /** {@inheritDoc} */ - public choose (): number { - if (this.pool.busy) { - return this.workerChoiceStrategy.choose() - } - if (!this.pool.full && this.pool.findFreeWorkerKey() === -1) { - return this.createWorkerCallback() - } - return this.workerChoiceStrategy.choose() - } - - /** {@inheritDoc} */ - public remove (workerKey: number): boolean { - return this.workerChoiceStrategy.remove(workerKey) - } -} diff --git a/src/pools/selection-strategies/selection-strategies-types.ts b/src/pools/selection-strategies/selection-strategies-types.ts index fb864c5b..ed6b0103 100644 --- a/src/pools/selection-strategies/selection-strategies-types.ts +++ b/src/pools/selection-strategies/selection-strategies-types.ts @@ -30,7 +30,7 @@ export const WorkerChoiceStrategies = Object.freeze({ export type WorkerChoiceStrategy = keyof typeof WorkerChoiceStrategies /** - * Pool tasks usage statistics requirements. + * Pool worker tasks usage statistics requirements. */ export interface RequiredStatistics { runTime: boolean diff --git a/src/pools/selection-strategies/worker-choice-strategy-context.ts b/src/pools/selection-strategies/worker-choice-strategy-context.ts index 1564e0f0..f6e0feab 100644 --- a/src/pools/selection-strategies/worker-choice-strategy-context.ts +++ b/src/pools/selection-strategies/worker-choice-strategy-context.ts @@ -1,7 +1,6 @@ import type { IPoolInternal } from '../pool-internal' import { PoolType } from '../pool-internal' import type { IPoolWorker } from '../pool-worker' -import { DynamicPoolWorkerChoiceStrategy } from './dynamic-pool-worker-choice-strategy' import type { IWorkerChoiceStrategy, RequiredStatistics, @@ -40,25 +39,6 @@ export class WorkerChoiceStrategyContext< this.setWorkerChoiceStrategy(workerChoiceStrategy) } - /** - * Gets the worker choice strategy instance specific to the pool type. - * - * @param workerChoiceStrategy - The worker choice strategy. - * @returns The worker choice strategy instance for the pool type. - */ - private getPoolWorkerChoiceStrategy ( - workerChoiceStrategy: WorkerChoiceStrategy = WorkerChoiceStrategies.ROUND_ROBIN - ): IWorkerChoiceStrategy { - if (this.pool.type === PoolType.DYNAMIC) { - return new DynamicPoolWorkerChoiceStrategy( - this.pool, - this.createWorkerCallback, - workerChoiceStrategy - ) - } - return getWorkerChoiceStrategy(this.pool, workerChoiceStrategy) - } - /** * Gets the worker choice strategy required statistics. * @@ -77,21 +57,30 @@ export class WorkerChoiceStrategyContext< workerChoiceStrategy: WorkerChoiceStrategy ): void { this.workerChoiceStrategy?.reset() - this.workerChoiceStrategy = - this.getPoolWorkerChoiceStrategy(workerChoiceStrategy) + this.workerChoiceStrategy = getWorkerChoiceStrategy( + this.pool, + workerChoiceStrategy + ) } /** - * Chooses a worker with the underlying selection strategy. + * Chooses a worker with the worker choice strategy. * * @returns The key of the chosen one. */ public execute (): number { + if ( + this.pool.type === PoolType.DYNAMIC && + !this.pool.full && + this.pool.findFreeWorkerKey() === -1 + ) { + return this.createWorkerCallback() + } return this.workerChoiceStrategy.choose() } /** - * Removes a worker in the underlying selection strategy internals. + * Removes a worker in the worker choice strategy internals. * * @param workerKey - The key of the worker to remove. * @returns `true` if the removal is successful, `false` otherwise. diff --git a/src/pools/thread/dynamic.ts b/src/pools/thread/dynamic.ts index 3333aa6e..33e42892 100644 --- a/src/pools/thread/dynamic.ts +++ b/src/pools/thread/dynamic.ts @@ -7,7 +7,7 @@ import { FixedThreadPool } from './fixed' * A thread pool with a dynamic number of threads, but a guaranteed minimum number of threads. * * This thread pool creates new threads when the others are busy, up to the maximum number of threads. - * When the maximum number of threads is reached, an event is emitted. If you want to listen to this event, use the pool's `emitter`. + * When the maximum number of threads is reached and workers are busy, an event is emitted. If you want to listen to this event, use the pool's `emitter`. * * @typeParam Data - Type of data sent to the worker. This can only be serializable data. * @typeParam Response - Type of response of execution. This can only be serializable data. diff --git a/tests/pools/selection-strategies/selection-strategies.test.js b/tests/pools/selection-strategies/selection-strategies.test.js index 7187a900..71a2819b 100644 --- a/tests/pools/selection-strategies/selection-strategies.test.js +++ b/tests/pools/selection-strategies/selection-strategies.test.js @@ -172,13 +172,11 @@ describe('Selection strategies test suite', () => { { workerChoiceStrategy: WorkerChoiceStrategies.WEIGHTED_ROUND_ROBIN } ) expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - .nextWorkerId + pool.workerChoiceStrategyContext.workerChoiceStrategy.nextWorkerId ).toBeUndefined() pool.setWorkerChoiceStrategy(WorkerChoiceStrategies.ROUND_ROBIN) expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - .nextWorkerId + pool.workerChoiceStrategyContext.workerChoiceStrategy.nextWorkerId ).toBe(0) // We need to clean up the resources after our test await pool.destroy() @@ -464,7 +462,7 @@ describe('Selection strategies test suite', () => { // if (process.platform !== 'win32') { // expect( // pool.workerChoiceStrategyContext.workerChoiceStrategy - // .workerChoiceStrategy.workerLastVirtualTaskTimestamp.size + // .workerLastVirtualTaskTimestamp.size // ).toBe(pool.workers.length) // } // We need to clean up the resources after our test @@ -500,18 +498,18 @@ describe('Selection strategies test suite', () => { './tests/worker-files/thread/testWorker.js' ) expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy + pool.workerChoiceStrategyContext.workerChoiceStrategy .workerLastVirtualTaskTimestamp ).toBeUndefined() pool.setWorkerChoiceStrategy(WorkerChoiceStrategies.FAIR_SHARE) - for (const workerKey of pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy.workerLastVirtualTaskTimestamp.keys()) { + for (const workerKey of pool.workerChoiceStrategyContext.workerChoiceStrategy.workerLastVirtualTaskTimestamp.keys()) { expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy.workerLastVirtualTaskTimestamp.get( + pool.workerChoiceStrategyContext.workerChoiceStrategy.workerLastVirtualTaskTimestamp.get( workerKey ).start ).toBe(0) expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy.workerLastVirtualTaskTimestamp.get( + pool.workerChoiceStrategyContext.workerChoiceStrategy.workerLastVirtualTaskTimestamp.get( workerKey ).end ).toBe(0) @@ -623,7 +621,7 @@ describe('Selection strategies test suite', () => { // TODO: Create a better test to cover `WeightedRoundRobinWorkerChoiceStrategy#choose` const promises = [] const maxMultiplier = - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy + pool.workerChoiceStrategyContext.workerChoiceStrategy .defaultWorkerWeight * 2 for (let i = 0; i < max * maxMultiplier; i++) { promises.push(pool.execute()) @@ -631,8 +629,8 @@ describe('Selection strategies test suite', () => { await Promise.all(promises) if (process.platform !== 'win32') { expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy - .workerChoiceStrategy.workersTaskRunTime.size + pool.workerChoiceStrategyContext.workerChoiceStrategy.workersTaskRunTime + .size ).toBe(pool.workers.length) } // We need to clean up the resources after our test @@ -674,29 +672,24 @@ describe('Selection strategies test suite', () => { './tests/worker-files/thread/testWorker.js' ) expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - .currentWorkerId + pool.workerChoiceStrategyContext.workerChoiceStrategy.currentWorkerId ).toBeUndefined() expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - .defaultWorkerWeight + pool.workerChoiceStrategyContext.workerChoiceStrategy.defaultWorkerWeight ).toBeUndefined() expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - .workersTaskRunTime + pool.workerChoiceStrategyContext.workerChoiceStrategy.workersTaskRunTime ).toBeUndefined() pool.setWorkerChoiceStrategy(WorkerChoiceStrategies.WEIGHTED_ROUND_ROBIN) expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - .currentWorkerId + pool.workerChoiceStrategyContext.workerChoiceStrategy.currentWorkerId ).toBe(0) expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - .defaultWorkerWeight + pool.workerChoiceStrategyContext.workerChoiceStrategy.defaultWorkerWeight ).toBeGreaterThan(0) - for (const workerKey of pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy.workersTaskRunTime.keys()) { + for (const workerKey of pool.workerChoiceStrategyContext.workerChoiceStrategy.workersTaskRunTime.keys()) { expect( - pool.workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy.workersTaskRunTime.get( + pool.workerChoiceStrategyContext.workerChoiceStrategy.workersTaskRunTime.get( workerKey ).runTime ).toBe(0) diff --git a/tests/pools/selection-strategies/worker-choice-strategy-context.test.js b/tests/pools/selection-strategies/worker-choice-strategy-context.test.js index 1f4b51cb..3f65fe82 100644 --- a/tests/pools/selection-strategies/worker-choice-strategy-context.test.js +++ b/tests/pools/selection-strategies/worker-choice-strategy-context.test.js @@ -23,9 +23,6 @@ const { const { WeightedRoundRobinWorkerChoiceStrategy } = require('../../../lib/pools/selection-strategies/weighted-round-robin-worker-choice-strategy') -const { - DynamicPoolWorkerChoiceStrategy -} = require('../../../lib/pools/selection-strategies/dynamic-pool-worker-choice-strategy') describe('Worker choice strategy context test suite', () => { const min = 1 @@ -109,11 +106,8 @@ describe('Worker choice strategy context test suite', () => { WorkerChoiceStrategies.ROUND_ROBIN ) expect(workerChoiceStrategyContext.workerChoiceStrategy).toBeInstanceOf( - DynamicPoolWorkerChoiceStrategy + RoundRobinWorkerChoiceStrategy ) - expect( - workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - ).toBeInstanceOf(RoundRobinWorkerChoiceStrategy) }) it('Verify that setWorkerChoiceStrategy() works with LESS_USED and fixed pool', () => { @@ -136,11 +130,8 @@ describe('Worker choice strategy context test suite', () => { WorkerChoiceStrategies.LESS_USED ) expect(workerChoiceStrategyContext.workerChoiceStrategy).toBeInstanceOf( - DynamicPoolWorkerChoiceStrategy + LessUsedWorkerChoiceStrategy ) - expect( - workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - ).toBeInstanceOf(LessUsedWorkerChoiceStrategy) }) it('Verify that setWorkerChoiceStrategy() works with LESS_BUSY and fixed pool', () => { @@ -163,11 +154,8 @@ describe('Worker choice strategy context test suite', () => { WorkerChoiceStrategies.LESS_BUSY ) expect(workerChoiceStrategyContext.workerChoiceStrategy).toBeInstanceOf( - DynamicPoolWorkerChoiceStrategy + LessBusyWorkerChoiceStrategy ) - expect( - workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - ).toBeInstanceOf(LessBusyWorkerChoiceStrategy) }) it('Verify that setWorkerChoiceStrategy() works with FAIR_SHARE and fixed pool', () => { @@ -190,11 +178,8 @@ describe('Worker choice strategy context test suite', () => { WorkerChoiceStrategies.FAIR_SHARE ) expect(workerChoiceStrategyContext.workerChoiceStrategy).toBeInstanceOf( - DynamicPoolWorkerChoiceStrategy + FairShareWorkerChoiceStrategy ) - expect( - workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - ).toBeInstanceOf(FairShareWorkerChoiceStrategy) }) it('Verify that setWorkerChoiceStrategy() works with WEIGHTED_ROUND_ROBIN and fixed pool', () => { @@ -217,10 +202,7 @@ describe('Worker choice strategy context test suite', () => { WorkerChoiceStrategies.WEIGHTED_ROUND_ROBIN ) expect(workerChoiceStrategyContext.workerChoiceStrategy).toBeInstanceOf( - DynamicPoolWorkerChoiceStrategy + WeightedRoundRobinWorkerChoiceStrategy ) - expect( - workerChoiceStrategyContext.workerChoiceStrategy.workerChoiceStrategy - ).toBeInstanceOf(WeightedRoundRobinWorkerChoiceStrategy) }) }) -- 2.34.1