Refactor the partitioning module to make it easier to introduce new algorithms #74275

wesleywiser · 2020-07-12T19:51:19Z

I've split the librustc_mir::monomorphize::partitioning module into a few files and introduced a Partitioner trait which allows us to decouple the partitioning algorithm from the code which integrates it into the query system. This should allow us to introduce new partitioning algorithms much more easily. I've also gone ahead and added a -Z flag to control which algorithm is used (currently there is only the default).

I left a few comments in places where things might be improved further.

r? @pnkfelix cc @rust-lang/wg-incr-comp

src/librustc_mir/monomorphize/partitioning/mod.rs

wesleywiser · 2020-07-13T00:50:58Z

Let's make sure this doesn't hurt performance.

@bors try @rust-timer queue

rust-timer · 2020-07-13T00:50:59Z

Awaiting bors try build completion

bors · 2020-07-13T00:51:13Z

⌛ Trying commit 42d7cf395b14b1b39b3050ffad364a1e15234f4a with merge 9b74ae087324a980952c4b760b1d7aab32b3ce65...

wesleywiser · 2020-07-13T00:57:02Z

src/librustc_mir/monomorphize/partitioning/merging.rs

+
+use crate::monomorphize::partitioning::PreInliningPartitioning;
+
+pub fn merge_codegen_units<'tcx>(


I pulled this out into a separate module since it seemed like we might want a few different merge algorithms to choose from.

I think this makes sense - I don't think it would prohibit us from having partitioning strategies which don't have a merging phase.

wesleywiser · 2020-07-13T01:07:55Z

src/librustc_mir/monomorphize/partitioning/mod.rs

@@ -0,0 +1,433 @@
+//! Partitioning Codegen Units for Incremental Compilation


This comment block is taken verbatim from its original location. Parts of it describe the partitioning process over all and parts are specific to the algorithm (now called DefaultPartitioner).

This could probably be left as is for now but eventually we should split it into two parts and move the bits about the algorithm into module documentation for the DefaultPartitioner algorithm.

wesleywiser · 2020-07-13T01:11:50Z

src/librustc_mir/monomorphize/partitioning/mod.rs

+        target_cgu_count: usize,
+    );
+
+    fn place_inlined_mono_items(


This name & signature was taken from the existing code. One that that was not immediately obvious to me was that this does a lot more than just add copies of #[inline] functions into CGUs. This function is primarily responsible for instantiating monomorphized functions in the modules that call them. (Perhaps that's what was meant by "inlined"?)

src/librustc_session/options.rs

bors · 2020-07-13T03:10:06Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 9b74ae087324a980952c4b760b1d7aab32b3ce65 (9b74ae087324a980952c4b760b1d7aab32b3ce65)

rust-timer · 2020-07-13T03:10:07Z

Queued 9b74ae087324a980952c4b760b1d7aab32b3ce65 with parent 9d09331, future comparison URL.

rust-timer · 2020-07-13T06:17:28Z

Finished benchmarking try commit (9b74ae087324a980952c4b760b1d7aab32b3ce65): comparison url.

bjorn3 · 2020-07-13T09:23:04Z

<0.1% speedup overall.

wesleywiser · 2020-07-13T10:26:35Z

r? @pnkfelix cc @rust-lang/wg-incr-comp

davidtwco

Current review comments notwithstanding, LGTM.

wesleywiser · 2020-07-15T01:55:33Z

Responded to review feedback so far.

bors · 2020-07-15T12:23:02Z

☔ The latest upstream changes (presumably #74214) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2020-07-19T11:03:13Z

☔ The latest upstream changes (presumably #74091) made this pull request unmergeable. Please resolve the merge conflicts.

richkadel

Sorry but I had to change this option's structure.

src/librustc_mir/monomorphize/partitioning/mod.rs

wesleywiser · 2020-07-23T21:36:33Z

Rebased.

@pnkfelix if you are interested in doing the review, I don't mind waiting for that, otherwise we can find someone else from wg-incr-comp to do the review.

pnkfelix · 2020-08-19T17:39:42Z

I left feedback over here on zulip (archive for those without zulip accounts); basically, I asked wesley to refactor the commit series so that the code shuffling is its own commit, so that the diff will better show what the actual changes are regarding the new trait.

This will allow us to prototype different partitioning schemes without adding a lot of extra conditionals everywhere.

wesleywiser · 2020-08-23T19:33:03Z

@pnkfelix I rebased and split out the trait into the first commit. The second commit has no functional changes, just moving the code into a new module.

pnkfelix · 2020-08-25T17:50:59Z

src/librustc_mir/monomorphize/partitioning.rs

+            internalization_candidates,
+        } = initial_partitioning;
+
+        let single_codegen_unit = initial_cgus.len() == 1;


Viewing in diff mode with whitespace changes hidden is a huge win here. :)

pnkfelix · 2020-08-25T17:59:24Z

@bors r+

bors · 2020-08-25T17:59:26Z

📌 Commit 98b943e has been approved by pnkfelix

bors · 2020-08-25T17:59:26Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

pietroalbini · 2020-08-25T18:52:05Z

@bors treeclosed-

bors · 2020-08-25T18:52:16Z

⌛ Testing commit 98b943e with merge 8ba2250...

bors · 2020-08-25T20:54:51Z

☀️ Test successful - checks-actions, checks-azure
Approved by: pnkfelix
Pushing 8ba2250 to master...

Three of the four methods in `DefaultPartitioning` are defined in `default.rs`. But `merge_codegen_units` is defined in a separate module, `merging`, even though it's less than 100 lines of code and roughly the same size as the other three methods. (Also, the `merging` module currently sits alongside `default`, when it should be a submodule of `default`, adding to the confusion.) In rust-lang#74275 this explanation was given: > I pulled this out into a separate module since it seemed like we might > want a few different merge algorithms to choose from. But in the three years since there have been no additional merging algorithms, and there is no mechanism for choosing between different merging algorithms. (There is a mechanism, `-Zcgu-partitioning-strategy`, for choosing between different partitioning strategies, but the merging algorithm is just one piece of a partitioning strategy.) This commit merges `merging` into `default`, making the code easier to navigate and read.

wesleywiser force-pushed the break_up_partitioning_rs branch from 981b094 to 42d7cf3 Compare July 12, 2020 19:52

bjorn3 reviewed Jul 12, 2020

View reviewed changes

src/librustc_mir/monomorphize/partitioning/mod.rs Outdated Show resolved Hide resolved

wesleywiser commented Jul 13, 2020

View reviewed changes

wesleywiser changed the title ~~[WIP] Refactor the partitioning module to make it easier to introduce new schemes~~ Refactor the partitioning module to make it easier to introduce new schemes Jul 13, 2020

wesleywiser changed the title ~~Refactor the partitioning module to make it easier to introduce new schemes~~ Refactor the partitioning module to make it easier to introduce new algorithms Jul 13, 2020

rust-highfive assigned pnkfelix Jul 13, 2020

davidtwco approved these changes Jul 13, 2020

View reviewed changes

wesleywiser force-pushed the break_up_partitioning_rs branch from 42d7cf3 to 6c7ab8d Compare July 15, 2020 01:55

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 15, 2020

wesleywiser force-pushed the break_up_partitioning_rs branch from 6c7ab8d to 2acd149 Compare July 18, 2020 15:17

richkadel reviewed Jul 19, 2020

View reviewed changes

src/librustc_mir/monomorphize/partitioning/mod.rs Outdated Show resolved Hide resolved

src/librustc_mir/monomorphize/partitioning/mod.rs Outdated Show resolved Hide resolved

wesleywiser force-pushed the break_up_partitioning_rs branch from 2acd149 to ee88fc1 Compare July 22, 2020 12:29

davidtwco approved these changes Jul 23, 2020

View reviewed changes

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2020

Add a CGU partitioning trait

8dea308

This will allow us to prototype different partitioning schemes without adding a lot of extra conditionals everywhere.

wesleywiser added 2 commits August 21, 2020 20:03

Split partitioning.rs into a module

5d501f9

Add partitioning -Z option to control which partitioning scheme is used

98b943e

wesleywiser force-pushed the break_up_partitioning_rs branch from ee88fc1 to 98b943e Compare August 22, 2020 00:04

pnkfelix reviewed Aug 25, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 25, 2020

bors merged commit 8ba2250 into rust-lang:master Aug 25, 2020

bors mentioned this pull request Aug 25, 2020

[Draft] Tools and tests to experimentally add more counters per function #75828

Closed

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the partitioning module to make it easier to introduce new algorithms #74275

Refactor the partitioning module to make it easier to introduce new algorithms #74275

wesleywiser commented Jul 12, 2020 •

edited

Loading

wesleywiser commented Jul 13, 2020

rust-timer commented Jul 13, 2020

bors commented Jul 13, 2020

wesleywiser Jul 13, 2020

davidtwco Jul 13, 2020

wesleywiser Jul 13, 2020

wesleywiser Jul 13, 2020 •

edited

Loading

bors commented Jul 13, 2020

rust-timer commented Jul 13, 2020

rust-timer commented Jul 13, 2020

bjorn3 commented Jul 13, 2020

wesleywiser commented Jul 13, 2020

davidtwco left a comment

wesleywiser commented Jul 15, 2020

bors commented Jul 15, 2020

bors commented Jul 19, 2020

richkadel left a comment

wesleywiser commented Jul 23, 2020

pnkfelix commented Aug 19, 2020 •

edited

Loading

wesleywiser commented Aug 23, 2020

pnkfelix Aug 25, 2020

pnkfelix commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 25, 2020

pietroalbini commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 25, 2020


		use crate::monomorphize::partitioning::PreInliningPartitioning;

		pub fn merge_codegen_units<'tcx>(

		@@ -0,0 +1,433 @@
		//! Partitioning Codegen Units for Incremental Compilation

Refactor the partitioning module to make it easier to introduce new algorithms #74275

Refactor the partitioning module to make it easier to introduce new algorithms #74275

Conversation

wesleywiser commented Jul 12, 2020 • edited Loading

wesleywiser commented Jul 13, 2020

rust-timer commented Jul 13, 2020

bors commented Jul 13, 2020

wesleywiser Jul 13, 2020

Choose a reason for hiding this comment

davidtwco Jul 13, 2020

Choose a reason for hiding this comment

wesleywiser Jul 13, 2020

Choose a reason for hiding this comment

wesleywiser Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

bors commented Jul 13, 2020

rust-timer commented Jul 13, 2020

rust-timer commented Jul 13, 2020

bjorn3 commented Jul 13, 2020

wesleywiser commented Jul 13, 2020

davidtwco left a comment

Choose a reason for hiding this comment

wesleywiser commented Jul 15, 2020

bors commented Jul 15, 2020

bors commented Jul 19, 2020

richkadel left a comment

Choose a reason for hiding this comment

wesleywiser commented Jul 23, 2020

pnkfelix commented Aug 19, 2020 • edited Loading

wesleywiser commented Aug 23, 2020

pnkfelix Aug 25, 2020

Choose a reason for hiding this comment

pnkfelix commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 25, 2020

pietroalbini commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 25, 2020

wesleywiser commented Jul 12, 2020 •

edited

Loading

wesleywiser Jul 13, 2020 •

edited

Loading

pnkfelix commented Aug 19, 2020 •

edited

Loading