feat: Add poolRespawn option to speed up incremental builds

[jantimon]

The thread-loader doubled the build duration for incremental builds because of the WorkerPool which is disposed after 500ms by default.

This pr tries adds poolRespawn:false to solve this problem similar to poolTimeout: Infinity. If set to false it will not respawn a worker pool once it was disposed.

So unlike poolTimeout: Infinity this feature allows to use the thread-loader only for the initial build.
The idea behind that is that the initial startup has to transpile all files but incremental builds usually transpile only a single file.

As this would have no negative impact for a production webpack-cli build we might even setpoolRespawn:false as default.
Please let me know if that would be okay for you so I can adjust this pr.

[jsf-clabot]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #52 into master will increase coverage by 2.96%.
The diff coverage is 71.42%.

@@            Coverage Diff            @@
##           master     #52      +/-   ##
=========================================
+ Coverage   16.94%   19.9%   +2.96%     
=========================================
  Files           7       7              
  Lines         425     432       +7     
  Branches       71      74       +3     
=========================================
+ Hits           72      86      +14     
+ Misses        310     305       -5     
+ Partials       43      41       -2

Impacted Files	Coverage Δ
src/workerPools.js	0% <0%> (ø)	⬆️
src/WorkerPool.js	28.98% <100%> (+5.83%)	⬆️
src/index.js	68.18% <50%> (-1.82%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfb6271...05b9e93. Read the comment docs.

[alexander-akait]

/cc @mistic for me looks good, what do you think?

[jantimon]

Cool if we change it to be the default it would also solve #26.

[mistic]

@jantimon just left a group of suggestions to make the implementation more clear and basically terminating the Worker pool after disposing when poolRespawn is false

[jantimon]

@mistic you are right we could reuse the terminated state and save a lot of code! 👍

I updated my pull request accordingly.

[mistic]

@jantimon I was writing down this when u wrote down your comment haha! Awesome, it's much better right now!

What do you think of, to finish, just do this instead of what is actually being done?

// Replace pitch() function on index.js
function pitch() {
  const options = loaderUtils.getOptions(this) || {};
  const workerPool = getPool(options);

  workerPool.run({
    loaders: this.loaders.slice(this.loaderIndex + 1).map((l) => {
      return {
        loader: l.path,
        options: l.options,
        ident: l.ident,
      };
    }),
    resource: this.resourcePath + (this.resourceQuery || ''),
    sourceMap: this.sourceMap,
    emitError: this.emitError,
    emitWarning: this.emitWarning,
    resolve: this.resolve,
    target: this.target,
    minimize: this.minimize,
    resourceQuery: this.resourceQuery,
    optionsContext: this.rootContext || this.options.context,
  }, this);
}

// Replace run() function on WorkerPool.js into WorkerPool class
run(data, pitchContext) {
  // Return right away if we have terminated and workerPool
  // is not configured to respawn
  if (this.terminated && !this.options.poolRespawn) {
    return;
  }

  // Indicate to loader runner that it should wait 
  // for an async operation calling the async from the 
  // given pitchContext
  const pitchContextAsyncCallback = pitchContext.async();

  // Build the callback for the workerPool queue call
  // when this job complete
  const runCallback = (err, r) => {
    if (r) {
      r.fileDependencies.forEach(d => this.addDependency(d));
      r.contextDependencies.forEach(d => this.addContextDependency(d));
    }
    if (err) {
      pitchContextAsyncCallback(err);
      return;
    }

    pitchContextAsyncCallback(null, ...r.result);
    return;
  };

  // Taking care of the time out, 
  // incrementing the active jobs 
  // and add the job to the workerPool queue
  if (this.timeout) {
    clearTimeout(this.timeout);
    this.timeout = null;
  }
  this.activeJobs += 1;
  this.poolQueue.push(data, runCallback);
}

[jantimon]

Is that code change related to this merge request?

If I understand it correct this.async() configures that the pitch loader works asynchronously and will call the callback once it is complete.

I am not sure why you would like to move this part away from the pitch loader and move it over to the responsibility of the WorkerPool.
What would be the advantage?

[mistic]

@jantimon the changes I proposed in the comment above they are just to encapsulate the code and run the logic inside the the run function, specially the one for:

if (this.terminated && !this.options.poolRespawn) {
    return;
}

[jantimon]

@mistic okay I understand what you mean.
I adjusted the code a little bit more.

Now it is only:

if (workerPool.terminated) {
  return;
}
const callback = this.async();
workerPool.run({

Please take a look if that might already be enough.
If you still think we should encapsulate that even further we might introduce a new helper function like 'isAbleToRun':

if (!workerPool.isAbleToRun()) {
  return;
}
const callback = this.async();
workerPool.run({

Imho that way we would not mix up the responsibilities.
What do you think?

[a-aslan]

I'm not sure this fixes #37.
I've applied the changes in this PR with no success: https://i.imgur.com/jO0qlMB.gifv

[alexander-akait]

@a-aslan feel free to investigate and send a PR with fix

[jantimon]

This is not related to #37.

[mistic]

@jantimon I like your second idea! If it was on me, I would rather prefer to run those checks inside the run function itself, but I'm fine with both! Be free to commit the updates!

[jantimon]

@mistic done :)

[mistic]

LGTM

[alexander-akait]

Something wrong with CI, need fix

[alexander-akait]

Need rebase too 👍

[jantimon]

Added tests and rebased to master :)

[alexander-akait]

@jantimon Great!