-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add in-order scheduling policy #10902
Conversation
O hai! Initial implementor here :) While I'm not opposed to the change, I think it should be configurable and definitely not the default. There are two things to consider here that may shed some light on why this was made this way:
Hope that explains the reasoning that the initial PR was lacking 🙂 |
Thank you @mjesun for these explanation. I'm open to making this configurable.
Metro does use workers because it doesn't know ahead of time if it has to transpile 1 or e.g. 5000 files and there might be more use cases where the worker are the consumer and producers of the tasks in which case RR can be problematic initially.
It's not limited to inline requires. Every worker has to pay for the JIT overhead. A "hot" worker has the code loaded and JITed. The latter only happens after the worker function was executed at least once.
That makes a lot of sense. I'll go ahead and introduce a new configuration option that allows to choose the scheduling mode. |
OK, it still doesn't make a 100% sense to me.
I'm not arguing against making this configurable and over different view points are probably the best reason why this should be configurable indeed :) |
That is correct as long as you have equivalent caches in all machines. If that is the case, then the building cost is the same no matter of the worker; and in this case I agree it is more beneficial to keep sending work to the same process as the cache is already warm in this one. But if the cache isn't globally equivalent (which I personally think is the common case here based on how it's used in Jest and Metro), then losing the "fat" worker that contains all the warmed cache leaves you having to rebuild it the entire cache from scratch again elsewhere. If the cache is distributed losing one worker would only kill Either way I get we're going to make it configurable, I'm just making the point to provide more information in case that helps 🙂 |
@cpojer would you mind having a look at the update PR in the new year? Thanks! |
Since it's opt-in I think this should be fine to merge 👍 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
The jest worker implementation today uses round robin to load balance the work across workers if sticky workers aren't used.
This is problematic if the result from the worker is in the critical path to create more work that can be scheduled on the threadpool. Metro, for example, uses the workers to transform the JavaScript files and extract its dependencies and then schedules the transformation of the dependencies. It's, therefore, critical that the first few files are transformed as quick as possible to build up a queue with enough work to use all workers.
The problem with the use of round robin is that jest assigns each task to a new worker of which each pays the cost of requiring modules (if inline-requires are used) and JITing the hot paths which, in the case of Metro, is a serious overhead
of a couple of seconds.
This diff adds the
in-order
scheduling policy and makes it configurable for clients (keeping round robin as the default). The first come policy assigns the task to the first free worker (starting with worker 0) or puts it in the queue of each worker to be picked up by the first available worker.Why call it
workerSchedulingPolicy
and not justschedulingPolicy
. My motivation is to make it clear that it does not affect the order in which tasks are scheduled, e.g. fifo vs lifo or a priority queue. AtaskSchedulingPolicy
might be something we want to add in the future.I tested this new scheduling with Metro:
Performance improved from 12.7s to 10.8s which is almost 2 seconds.
Main reason: Only one worker had to pay for the initialization cost
Outdated
The round robin implementation was added in #4921 by @mjesun but unfortunately without an in-detail explanation why it's superior. My understanding is that jest-worker is different from e.g. cluster that jest-worker knows how busy each worker is. A worker is either busy or idle. However, it then doesn't make a difference if all the work is performed by one worker or by different workers (assuming all are hot) as long as it uses the highest possible concurrency. This isn't true if sticky workers are used because they can then make use of caches but sticky workers aren't affected by this change
Test plan
Empty
Round Robin
In Order
Load
Round Robin
In Order