Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add option to disable run/wait time recording #518

Merged
merged 1 commit into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,8 @@ This class extends [`EventEmitter`][] from Node.js.
all Node.js versions higher than 14.6.0).
* `closeTimeout`: (`number`) An optional time (in milliseconds) to wait for the pool to
complete all in-flight tasks when `close()` is called. The default is `30000`
* `recordTiming`: (`boolean`) By default, run and wait time will be recorded
for the pool. To disable, set to `false`.

Use caution when setting resource limits. Setting limits that are too low may
result in the `Piscina` worker threads being unusable.
Expand Down
39 changes: 29 additions & 10 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ interface Options {
taskQueue? : TaskQueue,
niceIncrement? : number,
trackUnmanagedFds? : boolean,
closeTimeout?: number
closeTimeout?: number,
recordTiming?: boolean
}

interface FilledOptions extends Options {
Expand All @@ -191,7 +192,8 @@ interface FilledOptions extends Options {
useAtomics: boolean,
taskQueue : TaskQueue,
niceIncrement : number,
closeTimeout : number
closeTimeout : number,
recordTiming : boolean
}

const kDefaultOptions : FilledOptions = {
Expand All @@ -206,7 +208,8 @@ const kDefaultOptions : FilledOptions = {
taskQueue: new ArrayTaskQueue(),
niceIncrement: 0,
trackUnmanagedFds: true,
closeTimeout: 30000
closeTimeout: 30000,
recordTiming: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll prepare a PR to set this to false, as soon for v5

};

interface RunOptions {
Expand Down Expand Up @@ -590,8 +593,8 @@ class ThreadPool {
taskQueue : TaskQueue;
skipQueue : TaskInfo[] = [];
completed : number = 0;
runTime : RecordableHistogram;
waitTime : RecordableHistogram;
runTime? : RecordableHistogram;
waitTime? : RecordableHistogram;
needsDrain : boolean;
start : number = performance.now();
inProcessPendingMessages : boolean = false;
Expand All @@ -603,12 +606,16 @@ class ThreadPool {
constructor (publicInterface : Piscina, options : Options) {
this.publicInterface = publicInterface;
this.taskQueue = options.taskQueue || new ArrayTaskQueue();
this.runTime = createHistogram();
this.waitTime = createHistogram();

const filename =
options.filename ? maybeFileURLToPath(options.filename) : null;
this.options = { ...kDefaultOptions, ...options, filename, maxQueue: 0 };

if (this.options.recordTiming) {
this.runTime = createHistogram();
this.waitTime = createHistogram();
}

// The >= and <= could be > and < but this way we get 100 % coverage 🙃
if (options.maxThreads !== undefined &&
this.options.minThreads >= options.maxThreads) {
Expand Down Expand Up @@ -805,7 +812,7 @@ class ThreadPool {
break;
}
const now = performance.now();
this.waitTime.record(toHistogramIntegerNano(now - taskInfo.created));
this.waitTime?.record(toHistogramIntegerNano(now - taskInfo.created));
taskInfo.started = now;
workerInfo.postTask(taskInfo);
this._maybeDrain();
Expand Down Expand Up @@ -866,7 +873,7 @@ class ThreadPool {
(err : Error | null, result : any) => {
this.completed++;
if (taskInfo.started) {
this.runTime.record(toHistogramIntegerNano(performance.now() - taskInfo.started));
this.runTime?.record(toHistogramIntegerNano(performance.now() - taskInfo.started));
}
if (err !== null) {
reject(err);
Expand Down Expand Up @@ -956,7 +963,7 @@ class ThreadPool {

// TODO(addaleax): Clean up the waitTime/runTime recording.
const now = performance.now();
this.waitTime.record(toHistogramIntegerNano(now - taskInfo.created));
this.waitTime?.record(toHistogramIntegerNano(now - taskInfo.created));
taskInfo.started = now;
workerInfo.postTask(taskInfo);
this._maybeDrain();
Expand Down Expand Up @@ -1267,14 +1274,26 @@ class Piscina extends EventEmitterAsyncResource {
}

get waitTime () : any {
if (!this.#pool.waitTime) {
return null;
}

return createHistogramSummary(this.#pool.waitTime);
}

get runTime () : any {
if (!this.#pool.runTime) {
clydin marked this conversation as resolved.
Show resolved Hide resolved
return null;
}

return createHistogramSummary(this.#pool.runTime);
}

get utilization () : number {
if (!this.#pool.runTime) {
return 0;
}

// count is available as of Node.js v16.14.0 but not present in the types
const count = (this.#pool.runTime as RecordableHistogram & { count: number}).count;
if (count === 0) {
Expand Down
48 changes: 47 additions & 1 deletion test/histogram.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Piscina from '..';
import { test } from 'tap';
import { resolve } from 'path';

test('pool will maintain run and wait time histograms', async ({ equal, ok }) => {
test('pool will maintain run and wait time histograms by default', async ({ equal, ok }) => {
const pool = new Piscina({
filename: resolve(__dirname, 'fixtures/eval.js')
});
Expand All @@ -29,3 +29,49 @@ test('pool will maintain run and wait time histograms', async ({ equal, ok }) =>
equal(typeof runTime.min, 'number');
equal(typeof runTime.max, 'number');
});

test('pool will maintain run and wait time histograms when recordTiming is true', async ({ ok }) => {
const pool = new Piscina({
filename: resolve(__dirname, 'fixtures/eval.js'),
recordTiming: true
});

const tasks = [];
for (let n = 0; n < 10; n++) {
tasks.push(pool.runTask('42'));
}
await Promise.all(tasks);

const waitTime = pool.waitTime as any;
ok(waitTime);

const runTime = pool.runTime as any;
ok(runTime);
});

test('pool does not maintain run and wait time histograms when recordTiming is false', async ({ equal, fail }) => {
const pool = new Piscina({
filename: resolve(__dirname, 'fixtures/eval.js'),
recordTiming: false
});

const tasks = [];
for (let n = 0; n < 10; n++) {
tasks.push(pool.runTask('42'));
}
await Promise.all(tasks);

try {
pool.waitTime as any;
fail('Expected time recording disabled error');
} catch (error) {
equal(error.message, 'Time recording is disabled');
}

try {
pool.runTime as any;
fail('Expected time recording disabled error');
} catch (error) {
equal(error.message, 'Time recording is disabled');
}
});