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

worker optimization #93

Merged
merged 2 commits into from
Mar 9, 2016
Merged

worker optimization #93

merged 2 commits into from
Mar 9, 2016

Conversation

avikchaudhuri
Copy link
Contributor

Change the worker management to be dynamic rather than static. Instead of dividing all files among the available number of processes, we now send chunks of work to each free process, and when a process finishes its chunk of work it gets another chunk. This leads to overall much better perf for a large number of files with non-uniform processing times.

@@ -19,6 +19,7 @@ const fs = require('fs');
const path = require('path');

const availableCpus = require('os').cpus().length - 1;
const CHUNK_SIZE = 50;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I make this a config option?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this being a constant for the time being.

@fkling
Copy link
Contributor

fkling commented Mar 8, 2016

Sweet, thank you! Will merge your PRs tomorrow.... maybe this also makes it a bit easier to solve #87.

@avikchaudhuri
Copy link
Contributor Author

Not sure why the build failed, tests pass locally. Any way to kick off another build?

@cpojer
Copy link
Contributor

cpojer commented Mar 8, 2016

btw. I'd recommend switching to node-worker-farm: https://github.com/rvagg/node-worker-farm

@fkling
Copy link
Contributor

fkling commented Mar 8, 2016

I just restarted it. Sometimes testing the Babel option fails, no idea why.

@avikchaudhuri
Copy link
Contributor Author

Thanks! @cpojer Is it OK if we merge this PR now and we take on the work to use node-worker-farm later? I'm willing to investigate that option (looks really promising) but meanwhile I need some moar perf! :)

fkling added a commit that referenced this pull request Mar 9, 2016
@fkling fkling merged commit 975ae23 into facebook:master Mar 9, 2016
@fkling
Copy link
Contributor

fkling commented Mar 9, 2016

Released as v0.3.14. Thank you!

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

Successfully merging this pull request may close these issues.

None yet

4 participants