Skip to content
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
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"cacache": "^13.0.1",
"find-cache-dir": "^3.2.0",
"jest-worker": "^25.1.0",
"p-limit": "^2.2.2",
"schema-utils": "^2.6.4",
"serialize-javascript": "^2.1.2",
"source-map": "^0.6.1",
Expand Down
85 changes: 59 additions & 26 deletions src/TaskRunner.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os from 'os';

import pLimit from 'p-limit';
import Worker from 'jest-worker';
import serialize from 'serialize-javascript';

Expand All @@ -9,11 +10,15 @@ const workerPath = require.resolve('./worker');

export default class TaskRunner {
constructor(options = {}) {
this.taskGenerator = options.taskGenerator;
this.files = options.files;
this.cache = options.cache;
this.numberWorkers = TaskRunner.getNumberWorkers(options.parallel);
this.availableNumberOfCores = TaskRunner.getAvailableNumberOfCores(
options.parallel
);
}

static getNumberWorkers(parallel) {
static getAvailableNumberOfCores(parallel) {
// In some cases cpus() returns undefined
// https://github.com/nodejs/node/issues/19022
const cpus = os.cpus() || { length: 1 };
Expand All @@ -31,9 +36,18 @@ export default class TaskRunner {
return minify(task);
}

async run(tasks) {
if (this.numberWorkers > 1) {
this.worker = new Worker(workerPath, { numWorkers: this.numberWorkers });
async run() {
const { availableNumberOfCores, cache, files, taskGenerator } = this;

let concurrency = Infinity;

if (availableNumberOfCores > 0) {
// Do not create unnecessary workers when the number of files is less than the available cores, it saves memory
const numWorkers = Math.min(files.length, availableNumberOfCores);

concurrency = numWorkers;

this.worker = new Worker(workerPath, { numWorkers });

// Show syntax error from jest-worker
// https://github.com/facebook/jest/issues/8872#issuecomment-524822081
Expand All @@ -44,34 +58,53 @@ export default class TaskRunner {
}
}

return Promise.all(
tasks.map((task) => {
const enqueue = async () => {
let result;
const limit = pLimit(concurrency);
const scheduledTasks = [];

try {
result = await this.runTask(task);
} catch (error) {
result = { error };
for (const file of files) {
const enqueue = async (task) => {
let taskResult;

try {
taskResult = await this.runTask(task);
} catch (error) {
taskResult = { error };
}

if (cache.isEnabled() && !taskResult.error) {
taskResult = await cache.store(task, taskResult).then(
() => taskResult,
() => taskResult
);
}

task.callback(taskResult);

return taskResult;
};

scheduledTasks.push(
limit(() => {
const task = taskGenerator(file).next().value;

if (!task) {
// Something went wrong, for example the `cacheKeys` option throw an error
return Promise.resolve();
}

if (this.cache.isEnabled() && !result.error) {
return this.cache.store(task, result).then(
() => result,
() => result
if (cache.isEnabled()) {
return cache.get(task).then(
(taskResult) => task.callback(taskResult),
() => enqueue(task)
);
}

return result;
};

if (this.cache.isEnabled()) {
return this.cache.get(task).then((data) => data, enqueue);
}
return enqueue(task);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think plimit makes any guarantee about the order of execution of the enqueued functions. I think this means that enqueue(task) can be called in any order and therefore task.callback(taskResult) can be, too.

This seems like it has the potential to introduce non-determinism whenever parallelism is enabled. That is, if I run my webpack build twice, I expect it to produce the same outputs. This change seems to break that guarantee whenever extractComments is on since comment extraction depends on processing the results in a particular order.

It is important that this plugin is deterministic because nondeterministic builds needlessly invalidate long-term caches.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, we need sort properties of allExtractedComments here https://github.com/webpack-contrib/terser-webpack-plugin/blob/master/src/index.js#L509

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will fix it in near future

@alexander-akait alexander-akait Jan 29, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ok, I think you're right. Since comments are sorted and then de-duped after being collected in allExtractedComments, I think the output is deterministic.

})
);
}

return enqueue();
})
);
return Promise.all(scheduledTasks);
}

async exit() {
Expand Down
Loading