Take priority into account within the pending queue

[lqd]

This is the PR for the work discussed in this zulip thread and whose detailed description and some results are available here with graphs, summaries and raw data -- much of which was shown in the thread as well.

Units of works have a computed priority that is used in the dependency queue, so that higher priorities are dequeued sooner, as documented here.

This PR further applies that principle to the next step before being executed: if multiple pieces of work are waiting in the pending queue, we can sort that according to their priorities. Here as well, higher priorities should be scheduled sooner.

They are more often than not wider than pure chains of dependencies, and this should create more parallelism opportunities earlier in the pipeline: a high priority piece of work represents more future pieces of work down the line, and try to sustain CPU utilization longer (at the potential cost of this parallelism being distributed differently than today, between cargo invoking rustc and rustc's own codegen threads -- when applicable).

This is a scheduling tradeoff that behaves differently for each project, machine configuration, amount of available parallelism at a given point in time, etc, but seems to help more often than hinders: at low-core counts and with enough units of work to be done, so that there is jobserver token contention where choosing a "better" piece of work to work on next may be possible.

There's of course a bit of noise in the results linked above and 800 or so of the most popular crates.io crates is still a limited sample, but they're mostly meant to show a hopefully positive trend: while there are improvements and regressions, that trend looks to be more positive than negative, with the wins being more numerous and with higher amplitudes than the corresponding losses.

(A report on another scheduling experiment -- a superset of this PR, where I also simulate users manually giving a higher priority to syn, quote, serde_derive -- is available here and also improves this PR's results: the regressions are decreased in number and amplitude, whereas the improvements are bigger and more numerous. So that could be further work to iterate upon this one)

Since this mostly applies to clean builds, for low core counts, and with a sufficient number of dependencies to have some items in the pending queue, I feel this also applies well to CI use-cases (esp. on the free tiers).

Somewhat reassuring as well, and discussed in the thread but not in the report: I've also tried to make sure cargo and bootstrapping rustc are not negatively affected. cargo saw some improvements, and bootstrap stayed within today's variance of +/- 2 to 3%. Similarly, since 3y old versions of some tokio crates (0.2.0-alpha.1) were the most negatively affected, I've also checked that recent tokio versions (1.19) are not disproportionately impacted: their simple readme example, the more idiomatic mini-redis sample, and some of my friends' tokio projects were either unaffected or saw some interesting improvements.

And here's a cargo check -j2 graph to liven up this wall of text:

---

I'm not a cargo expert so I'm not sure whether it would be preferable to integrate priorities deeper than just the dependency queue, and e.g. have Units contain a dedicated field or similar. So in the meantime I've done the simplest thing: just sort the pending queue and ask the units' priorities to the dep queue.

We could just as well have the priority recorded as part of the pending queue tuples themselves, or have that be some kind of priority queue/max heap instead of a Vec.

Let me know which you prefer, but it's in essence a very simple change as-is.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information.

[weihanglo]

The change is simple but straightforward. I cannot think of anything blocking this from getting merged. Thank you for putting so much time on these benchmarks!

[weihanglo]

Would it be better with sort_by_cached_key? Supposed cargo does some hashes here.

(Benchmark needed, though)

[Eh2406]

If we going to do a sort here anyway, then self.queue.dequeue() should not guarantee what order they items are returned in. In fact it should probably just return an iterator.

Also if we sort from lowest priority to highest priority, then when remove an item we can use the .pop() where we are now using self.pending_queue.remove(0);

[lqd]

@weihanglo Yeah, this is why I also mentioned whether you wanted a different structure where the priority would be part of the data stored in the pending queue.

Ultimately it won't make a huge difference, especially with the allocation involved in caching, but those Units are indeed pretty big and their hashing may not be super cheap, so it should still improve a bit indeed.

I've pushed a change into a sort_by_cached_key call.

[lqd]

@Eh2406 the sort will only apply if there are units in the pending queue here: so IIUC there can be cases (maybe when there are always jobserver tokens available, or at j1) where the units could be processed one by one, in the order in which they are dequeued from the dependency queue. For these cases, we'd still need to keep the max-priority ordering in self.queue.dequeue(), right ?

I've also pushed your suggestion to pull the high priority items from the end of the queue. (We could probably use the .pop() as a replacement for the !is_empty() check at the top of the loop, but I think the let chains to do that are still unstable)

[weihanglo]

Yep. Using something like priority queue might be better. It is somehow a bit weird here
that we push the highest priority item from self.queue.dequeue(), and then immediately sort pending_queue in ascending order.

Anyway, we can always do it in a follow-up PR. I feel like we can merge it as-is. It would be wonderful if you can do another benchmark before r+. Thank you!

[lqd]

Sure, what kind of benchmark would you like to see ?

[weihanglo]

No particular one. The same benchmark you did is good enough. Just want to leave a trace for how much performance gains Cargo benefits from this commit.

[lqd]

It is somehow a bit weird here that we push the highest priority item from self.queue.dequeue(), and then immediately sort pending_queue in ascending order.

It is, and if we want to keep a Vec for now instead of a max-heap, we can insert the unit directly at the correct index instead.

No particular one. The same benchmark you did is good enough

In the report there are 800 benchmarks (× 7 runs × 2 versions of cargo × 3 parallelism settings × 2 build modes), so re-running everything is not going to be possible soon unfortunately because it takes multiple days. I'll re-run some important ones, on a few different machines, e.g. tokio's mini-redis (I've been benchmarking that when pushing to this branch today, and it's still showing -9% at j4/6 for example) and some of the most positively and negatively affected crates.

I'll do those 2 things tomorrow.

[lqd]

Since we're keeping the pending queue as a vec for now, I've added the priority to its tuples: that removes the need for push/sort/hashing, we can more easily and efficiently insert the items at the correctly sorted position. Let me know if you like it better.

@weihanglo here are some benchmarks you asked for as well: I've re-ran some of the highest relative wins and losses from the report, as well as cargo, and tokio's mini-redis (to contrast with the old version of a tokio crate we see in the losses).

So here are the hyperfine results of 10 clean runs (with 2 additional warmup runs), for check and debug builds,

• on linux, 24C/48T AMD EPYC 7401P: from j2 to j12
• on mac, 8C kinda, M1: from j2 to j8
• on windows, 4C/8T Intel i7 7700: from j2 to j8

for the crates:

• one of the highest relative wins (at cargo build -j2 in the report), publicsuffix-2.1.1: linux-amd, osx-m1, win-intel.
• one of the biggest relative losses (at cargo check -j4 in the report), tokio-reactor-0.2.0-alpha.1: linux-amd, osx-m1, win-intel.
• tokio's mini-redis with tokio 1.19: linux-amd, osx-m1, win-intel.
• cargo: linux-amd, osx-m1, win-intel.

(In these results, cargo-sorted is this PR, while cargo-baseline is cargo master from this morning.)

[weihanglo]

@rfcbot merge

This keeps the pending_queue of unit jobs sorted, in order to make sure cargo always schedules a pending item with the highest priority first. More complete benchmarks are in this zulip discussion. A benchmark for the latest commit 4ffe98a is tracked here.

I feel like this PR just respects the existing priority scheduling mechanism. It doesn't invent anything new, so it might be safe to merge (at least the benchmark result looks generally good).

One other thing to enhance is using some data structure other than Vec for pending_queue. We can try it out within this PR, but I doubg it affects perf much more that this PR so far.

[rfcbot]

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Eh2406]

This max_by_key is unnecessary as we are removing all and then sorting. If we change it to .next(), only the test will need to be updated.

[lqd]

I can remove it if you want ? It doesn't seem to hurt today, and makes the order at least deterministic.

[Eh2406]

I got nerd sniped, but it doesn't matter. Leave it bee.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Eh2406]

@bors r+

[bors]

📌 Commit 4ffe98a has been approved by Eh2406

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4ffe98a with merge 0825039...

[bors]

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 0825039 to master...

[SimonSapin]

How are priorities determined? Can end users influence them?

[weihanglo]

Can end users influence them?

Unfortunately, no.
It is under consideration but may need more experiments. For today, you could use cargo build --timings to find the bottleneck of your build. And then prioritize manually by building that crate in advance via cargo build -p <slow-crate>.

How are priorities determined?

Still naive. Basically, every unit gets a same fixed cost¹, and the priority of each is a sum of all the cost of its children plus its own². We're welcome for more real world data and experiments!

Footnotes

1. https://github.com/rust-lang/cargo/blob/2d04bcd1b0d6f40001a7b1762f78842851d0b334/src/cargo/core/compiler/job_queue.rs#L534-L538 ↩

2. https://github.com/rust-lang/cargo/blob/996a6363ce4b9109d4ca757407dd6dcb4805c86f/src/cargo/util/dependency_queue.rs#L105-L112 ↩