Prefetch some queries used by the metadata encoder #67888

Zoxc · 2020-01-05T04:47:58Z

This brings the time for metadata encoding and writing for syntex_syntax from 1.338s to 0.997s with 6 threads in non-incremental debug mode.

r? @Mark-Simulacrum

Zoxc · 2020-01-11T03:52:00Z

I did some more tuning and brought the time for metadata encoding and writing down to 0.561.

@michaelwoerister Do you know why the incremental test failed here given that this PR doesn't change dependencies?

michaelwoerister · 2020-01-13T09:37:46Z

@michaelwoerister Do you know why the incremental test failed here given that this PR doesn't change dependencies?

I don't. The change doesn't look like should break that test.

Zoxc · 2020-01-13T13:27:42Z

Looks like we don't check queries which did not execute, and this caused some promoted_mir queries to execute.

Mark-Simulacrum

I am feeling uncertain about this PR. I would appreciate getting some wider feedback (not sure from who, maybe @michaelwoerister)... it feels like while it does give us some good wins, it feels somewhat fragile (i.e., depends on how metadata encoding works pretty closely).

I would rather see us explore making metadata encoding itself more parallel -- IIRC, the basic idea with encoding is a bunch of arrays representing trait impls, MIR, etc. -- maybe we can instead make constructing those be parallel?

Mark-Simulacrum · 2020-01-20T16:30:51Z

src/librustc_metadata/rmeta/encoder.rs

+        i = self.position();
+        let exported_symbols = self.tcx.exported_symbols(LOCAL_CRATE);
+        let exported_symbols = self.encode_exported_symbols(&exported_symbols);
+        let exported_symbols_bytes = self.position() - i;


Could this commit be fleshed out with some description of why this is done? (i.e., in the commit message ideally)?

Right now it looks like presumably it's to make sure the exported_symbols query can fallback on the parallel MIR optimization in the last commit... but I'm not sure.

Mark-Simulacrum · 2020-01-20T16:31:37Z

src/librustc_metadata/rmeta/encoder.rs

+                            tcx.promoted_mir(def_id);
+                        })
+                    },
+                    || tcx.exported_symbols(LOCAL_CRATE),


Ah, so was this why the previous commit moved exported symbols later?

Yes. It's moved later to give more time for prefetching to happen.

Zoxc · 2020-01-21T12:46:54Z

I would rather see us explore making metadata encoding itself more parallel -- IIRC, the basic idea with encoding is a bunch of arrays representing trait impls, MIR, etc. -- maybe we can instead make constructing those be parallel?

I'd like to remove the existing metadata and all related code and instead use the incremental query cache for both metadata and incremental compilation so I don't really want to put any effort into refactoring the existing code.

Mark-Simulacrum · 2020-01-21T13:04:24Z

Should we then not land this either? Feels like the 0.4 second win is nice but not huge, and presumably would be less in incremental mode (more data to load?).

I feel like replacing metadata with incremental query cache is a pretty far reaching goal though -- maybe worth trying to polish metadata into better shape in the mean time? But I can see us not wanting to spend time on it. Obviously out of scope for this PR.

I guess I'm not opposed to landing this PR -- but I would like to see the first review comment addressed (expanding on the commit).

michaelwoerister · 2020-01-22T12:35:49Z

Here are some thoughts:

The changes seem relatively safe as far as correctness is concerned (although I would add a comment that tcx.dep_graph.with_ignore() is only safe because query results aren't accessed).
Generally, doing prefetching in a parallel setting also makes sense.
However, the PR does add a bit of complexity and duplicates some logic, and
we only have one performance number of one crate (that is known to be a bit of an edge case) in one compilation mode from a single machine without context (e.g. by what percentage did the end-to-end compile time for the crate change). So we don't have a lot of data to base this decision on; and we won't get more even after merging (at least not from perf.rlo).

So I'm on the fence on whether I think this is worth the trouble or not. Since the changes are safe and can be easily reverted, I'd say it's OK to merge but maybe with more comments, i.e.:

marking the duplicated logic as such and referring to the respective other occurrences that need to be kept in sync).
adding a comment that this prefetching is non-essential and can just be removed if it causes trouble or has detrimental effects.
adding a comment about tcx.dep_graph.with_ignore()

Mark-Simulacrum · 2020-02-02T14:48:39Z

I am also not opposed to merging with more comments.

joelpalmer · 2020-03-09T10:53:39Z

Triaged

Zoxc · 2020-03-14T13:16:26Z

I added some comments and make the code use assert_ignored instead of with_ignore.

Mark-Simulacrum · 2020-03-14T20:30:25Z

The changes look reasonable, but I cannot review the prefetching of the MIR bodies, as I'm not familiar enough with the code that'll be using that prefetching later on (nor with the relevant queries). I'm a little worried by the amount of code that is needed for prefetching there, too, particularly as it seems likely to not get updated over time (given the complex conditionals especially) to fit exactly what we need.

With that in mind, let's try r? @matthewjasper perhaps? I'm not sure if you're the best person for the optimized/promoted MIR queries, which seem to be dominant in that convoluted code.

bors · 2020-03-19T03:36:15Z

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

matthewjasper · 2020-03-19T16:38:26Z

@bors r+

bors · 2020-03-19T16:38:27Z

📌 Commit 027c8d9 has been approved by matthewjasper

@Mark-Simulacrum

…sper Prefetch some queries used by the metadata encoder This brings the time for `metadata encoding and writing` for `syntex_syntax` from 1.338s to 0.997s with 6 threads in non-incremental debug mode. r? @Mark-Simulacrum

@ghost

Rollup of 8 pull requests Successful merges: - rust-lang#67888 (Prefetch some queries used by the metadata encoder) - rust-lang#69934 (Update the mir inline costs) - rust-lang#69965 (Refactorings to get rid of rustc_codegen_utils) - rust-lang#70054 (Build dist-android with --enable-profiler) - rust-lang#70089 (rustc_infer: remove InferCtxt::closure_sig as the FnSig is always shallowly known.) - rust-lang#70092 (hir: replace "items" terminology with "nodes" where appropriate.) - rust-lang#70138 (do not 'return' in 'throw_' macros) - rust-lang#70151 (Update stdarch submodule) Failed merges: - rust-lang#70074 (Expand: nix all fatal errors) r? @ghost

rust-highfive assigned Mark-Simulacrum Jan 5, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2020

Zoxc force-pushed the metadata-prefetch branch from 3544a47 to bb2104d Compare January 5, 2020 04:56

This comment has been minimized.

Sign in to view

Zoxc force-pushed the metadata-prefetch branch from bb2104d to 9ee86c3 Compare January 11, 2020 03:49

This comment has been minimized.

Sign in to view

Zoxc force-pushed the metadata-prefetch branch from 9ee86c3 to 8b62655 Compare January 13, 2020 15:23

Zoxc mentioned this pull request Jan 14, 2020

More parallel tweaks #68218

Closed

Mark-Simulacrum reviewed Jan 20, 2020

View reviewed changes

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

Zoxc force-pushed the metadata-prefetch branch from 8b62655 to 24cd6cd Compare March 14, 2020 13:15

rust-highfive assigned matthewjasper and unassigned Mark-Simulacrum Mar 14, 2020

Zoxc added 5 commits March 19, 2020 15:12

Prefetch queries used by the metadata encoder

6cd0dca

Encode exported symbols last

1a34cbc

Prefetch exported symbols

03af82b

Make the timer more verbose

3d59c0e

Make metadata prefetching more accurate

a2bca90

Zoxc added 2 commits March 19, 2020 15:22

Add some comments

801e442

Use assert_ignored when encoding metadata

027c8d9

Zoxc force-pushed the metadata-prefetch branch from 24cd6cd to 027c8d9 Compare March 19, 2020 14:24

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2020

Dylan-DPC-zz mentioned this pull request Mar 19, 2020

Rollup of 9 pull requests #70169

Closed

Centril mentioned this pull request Mar 21, 2020

Rollup of 8 pull requests #70211

Merged

bors merged commit 9adfb18 into rust-lang:master Mar 21, 2020

Zoxc deleted the metadata-prefetch branch March 21, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefetch some queries used by the metadata encoder #67888

Prefetch some queries used by the metadata encoder #67888

Zoxc commented Jan 5, 2020 •

edited

Loading

This comment has been minimized.

Zoxc commented Jan 11, 2020

This comment has been minimized.

michaelwoerister commented Jan 13, 2020

Zoxc commented Jan 13, 2020

Mark-Simulacrum left a comment

Mark-Simulacrum Jan 20, 2020

Mark-Simulacrum Jan 20, 2020

Zoxc Jan 21, 2020

Zoxc commented Jan 21, 2020

Mark-Simulacrum commented Jan 21, 2020

michaelwoerister commented Jan 22, 2020

Mark-Simulacrum commented Feb 2, 2020

joelpalmer commented Mar 9, 2020

Zoxc commented Mar 14, 2020

Mark-Simulacrum commented Mar 14, 2020

bors commented Mar 19, 2020

matthewjasper commented Mar 19, 2020

bors commented Mar 19, 2020

Prefetch some queries used by the metadata encoder #67888

Prefetch some queries used by the metadata encoder #67888

Conversation

Zoxc commented Jan 5, 2020 • edited Loading

This comment has been minimized.

Zoxc commented Jan 11, 2020

This comment has been minimized.

michaelwoerister commented Jan 13, 2020

Zoxc commented Jan 13, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Mark-Simulacrum Jan 20, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Jan 20, 2020

Choose a reason for hiding this comment

Zoxc Jan 21, 2020

Choose a reason for hiding this comment

Zoxc commented Jan 21, 2020

Mark-Simulacrum commented Jan 21, 2020

michaelwoerister commented Jan 22, 2020

Mark-Simulacrum commented Feb 2, 2020

joelpalmer commented Mar 9, 2020

Zoxc commented Mar 14, 2020

Mark-Simulacrum commented Mar 14, 2020

bors commented Mar 19, 2020

matthewjasper commented Mar 19, 2020

bors commented Mar 19, 2020

Zoxc commented Jan 5, 2020 •

edited

Loading