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 `poolRespawn` option to speed up incremental builds #52

Merged
merged 1 commit into from Dec 21, 2018

Conversation

Projects
None yet
5 participants
@jantimon
Copy link
Contributor

jantimon commented Dec 19, 2018

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

This comment has been minimized.

Copy link

jsf-clabot commented Dec 19, 2018

CLA assistant check
All committers have signed the CLA.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 19, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Dec 19, 2018

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

@jantimon

This comment has been minimized.

Copy link
Contributor

jantimon commented Dec 19, 2018

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

@mistic
Copy link
Contributor

mistic left a comment

@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

Show resolved Hide resolved src/index.js Outdated
Show resolved Hide resolved src/WorkerPool.js Outdated
Show resolved Hide resolved src/WorkerPool.js Outdated
Show resolved Hide resolved src/WorkerPool.js Outdated
@jantimon

This comment has been minimized.

Copy link
Contributor

jantimon commented Dec 19, 2018

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

I updated my pull request accordingly.

@mistic

This comment has been minimized.

Copy link
Contributor

mistic commented Dec 19, 2018

@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

This comment has been minimized.

Copy link
Contributor

jantimon commented Dec 19, 2018

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

This comment has been minimized.

Copy link
Contributor

mistic commented Dec 20, 2018

@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 jantimon force-pushed the jantimon:master branch from 2d92a7f to a0a7faa Dec 20, 2018

@jantimon

This comment has been minimized.

Copy link
Contributor

jantimon commented Dec 20, 2018

@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

This comment has been minimized.

Copy link

a-aslan commented Dec 20, 2018

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

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Dec 20, 2018

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

@jantimon

This comment has been minimized.

Copy link
Contributor

jantimon commented Dec 20, 2018

This is not related to #37.

@mistic

This comment has been minimized.

Copy link
Contributor

mistic commented Dec 20, 2018

@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 jantimon force-pushed the jantimon:master branch from a0a7faa to 64a2772 Dec 20, 2018

@jantimon

This comment has been minimized.

Copy link
Contributor

jantimon commented Dec 20, 2018

@mistic done :)

@mistic

mistic approved these changes Dec 20, 2018

Copy link
Contributor

mistic left a comment

LGTM

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Dec 20, 2018

Something wrong with CI, need fix

@jantimon jantimon force-pushed the jantimon:master branch from 64a2772 to f246e2f Dec 20, 2018

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Dec 20, 2018

Need rebase too 👍

@jantimon jantimon force-pushed the jantimon:master branch 3 times, most recently from fdfea8f to 4c43159 Dec 20, 2018

@jantimon jantimon force-pushed the jantimon:master branch from 4c43159 to 05b9e93 Dec 20, 2018

@jantimon

This comment has been minimized.

Copy link
Contributor

jantimon commented Dec 20, 2018

Added tests and rebased to master :)

@evilebottnawi evilebottnawi merged commit 76535bf into webpack-contrib:master Dec 21, 2018

5 checks passed

codecov/patch 71.42% of diff hit (target 16.94%)
Details
codecov/project 19.9% (+2.96%) compared to bfb6271
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Dec 21, 2018

@jantimon Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment