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

PriorityQueue implementation for jest-worker #10921

Merged
merged 7 commits into from
Dec 26, 2020

Conversation

MichaReiser
Copy link
Contributor

Summary

The current jest-worker implementation processes tasks in FIFO ordering. This assumes that all tasks are equally important which isn't always given.

This PR extracts a TaskQueue interface, extracts the existing implementation as FifoQueue, adds a new PriorityQueue implementations and allows customizing the used queue implementation when creating the JestWorker.

A TaskQueue offers two operations:

  • enqueue(workerId: ?number): Add a new task to the queue of the specified worker or for all workers if the workerId is absent
  • dequeue(workerId: number): Return the next task to process for the specified worker id.

The PriorityQueue uses a MinHeap that has log(n) complexity for enqueue and O(1) complexity for the dequeue implementation. The implementation uses a shared and n worker specific queues to support sticky workers. The implementation returns the tasks in the ordering according to their priority. Tasks with equal priority are returned in any order except that sticky-tasks are returned before tasks queued in the shared queue.

This diff extracts the current queue implementation into the FifoQueue class and refactors the implementation to reduce the memory footprint. The current implementation added non-worker specific tasks to the queue of each worker. The result is that there are n * tasks queue entries where n is the number of workers for shared tasks. Furthermore, dequeuing required to iterate through all tasks from the top until a task that hasn't been processed is found. The refactored implementation uses a shared queue and a separate queue for each worker. Shared tasks are only enqueued in the shared queue. Each task can only be part of one queue making it redundant to find the first non-processed task during dequeue.

Test plan

Unit tests for the new queue implementations. Existing integration tests pass with the refactored FIFO queue implementation.

Empty

FIFO Current

---------------------------------------------------------------------------
total worker-farm: { wFGT: 85202.2141059637, wFPT: 85074.46953901649 }
total jest-worker: { jWGT: 24398.66391697526, jWPT: 24248.10139900446 }
---------------------------------------------------------------------------
% improvement over 100000 calls (global time): 71.3638146930885
% improvement over 100000 calls (processing time): 71.49779301546641

FIFO Refactored

---------------------------------------------------------------------------
total worker-farm: { wFGT: 88682.86321604252, wFPT: 88576.52663603425 }
total jest-worker: { jWGT: 24277.916941016912, jWPT: 24127.625441014767 }
---------------------------------------------------------------------------
% improvement over 100000 calls (global time): 72.62389140292767
% improvement over 100000 calls (processing time): 72.76070042782725

loadTest

FIFO Current

total worker-farm: { wFGT: 181426.06709498167, wFPT: 181314.26368799806 }
total jest-worker: { jWGT: 153297.50806596875, jWPT: 153158.72005698085 }
---------------------------------------------------------------------------
% improvement over 100000 calls (global time): 15.504144183584613
% improvement over 100000 calls (processing time): 15.528587248638475

FIFO Refactored

---------------------------------------------------------------------------
total worker-farm: { wFGT: 177797.00552800298, wFPT: 177702.07569798827 }
total jest-worker: { jWGT: 146847.8964600265, jWPT: 146713.39515596628 }
---------------------------------------------------------------------------
% improvement over 100000 calls (global time): 17.406991178544903
% improvement over 100000 calls (processing time): 17.438558565116864

@SimenB
Copy link
Member

SimenB commented Dec 23, 2020

@MichaReiser @mjesun is this waiting for me? 😅

@MichaReiser
Copy link
Contributor Author

@SimenB would be great if you could take a look at it to get someone else's opinion on the changes and if this is something that would be beneficial for jest-worker.

@SimenB
Copy link
Member

SimenB commented Dec 23, 2020

I assumed this was some FB thing that you needed and it'd go through without me having to do anything except publishing 😅 From skimming I think this looks good, especially as there's no change in default behavior. And the motivation for the new feature seems sound to me 👍 And of course the improved benchmarks you posted are awesome

packages/jest-worker/src/PriorityQueue.ts Outdated Show resolved Hide resolved
packages/jest-worker/src/PriorityQueue.ts Outdated Show resolved Hide resolved
packages/jest-worker/src/__performance_tests__/test.js Outdated Show resolved Hide resolved
queue.enqueue(createQueueChildMessage({priority}));
}

priorities.sort((a, b) => a - b);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
priorities.sort((a, b) => a - b);
priorities.sort();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The override is required because JavaScript does by default a lexicographical sort.

The sort() method sorts the elements of an array in place and returns the sorted array. The default sort order is ascending, built upon converting the elements into strings, then comparing their sequences of UTF-16 code units values. MDN

[10, 3, 4, 8, 2, 9, 7, 1, 2, 6, 5].sort()
Array(11) [ 1, 10, 2, 2, 3, 4, 5, 6, 7, 8, … ]

Copy link
Member

Choose a reason for hiding this comment

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

JS strikes again 😀

packages/jest-worker/src/index.ts Show resolved Hide resolved
@MichaReiser
Copy link
Contributor Author

Thanks @SimenB for your thoughtful review!

I assumed this was some FB thing that you needed and it'd go through without me having to do anything except publishing

It did start as part of some work on Metro but then turned out that switching to a priority queue does not improve the performance as much as we hoped for. I then decided to clean up the implementation because in my spare time because I believed the separation of concerns (extracting the task queue) is worth it and a priority queue might come in handy for someone else (or we might need it in the end).

@MichaReiser
Copy link
Contributor Author

I resolved the conflicts of this PR and added the changelog entry. Should now be ready to merge.

@SimenB SimenB merged commit 0ab070c into jestjs:master Dec 26, 2020
@SimenB
Copy link
Member

SimenB commented Dec 26, 2020

Thanks!

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants