Deduplicate obligations in `opt_normalize_projection_type` #91186

the8472 · 2021-11-24T18:41:55Z

This adds a second deduplication site (in addition to the original one at impl_or_trait_obligations) in opt_normalize_projection_type since that's where the OOMing allocations occured in #74456.

Fixes an OOM and compile time blowup in the reduced test-case on that issue.

Since it only touches one place where obligations are processed it might not fix all of these blowups. If more places need deduplication then either they also need to have the vec locally wrapped in ObligationsDedup or we need to start plumbing the deduplication or a sorted set all the way through the APIs that currently use Vec.

perf results from original PR: #90913
The deep-vector debug incr-unchanged regression is most likely spurious due to incremental verification noise, so the perf impact is mostly positive.

rust-highfive · 2021-11-24T18:41:58Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_infer/src/traits/mod.rs

compiler/rustc_trait_selection/src/traits/project.rs

Aaron1011 · 2021-11-26T01:13:15Z

@bors try @rust-timer queue

rust-timer · 2021-11-26T01:13:16Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-26T01:13:23Z

⌛ Trying commit 34812ba124bf254ef86abf1b61c5801d58b2375e with merge 1937cf62d0cb08000a49346c146830a7de817de3...

bors · 2021-11-26T02:48:17Z

☀️ Try build successful - checks-actions
Build commit: 1937cf62d0cb08000a49346c146830a7de817de3 (1937cf62d0cb08000a49346c146830a7de817de3)

rust-timer · 2021-11-26T02:48:19Z

Queued 1937cf62d0cb08000a49346c146830a7de817de3 with parent dd549dc, future comparison URL.

rust-timer · 2021-11-26T05:08:18Z

Finished benchmarking commit (1937cf62d0cb08000a49346c146830a7de817de3): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -6.7% on full builds of deeply-nested)
Very large regression in instruction counts (up to 5.4% on incr-unchanged builds of inflate)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

the8472 · 2021-11-26T18:22:37Z

Mostly the same results, except a different incr-unchanged one showing up, just making it more likely that it's incremental verification noise.

the8472 · 2021-12-07T20:11:54Z

ping @estebank

bors · 2021-12-15T07:02:29Z

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

the8472 · 2021-12-15T22:05:04Z

Rerunning perf since #90423 landed.

@bors try @rust-timer queue

rust-timer · 2021-12-15T22:05:05Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-12-15T22:05:12Z

⌛ Trying commit df24a7fe0c8de8459b11d2022276c9222f8c1191 with merge a00e9c1413f84459311a0b7c905264d28c199c08...

rust-timer · 2021-12-15T23:33:20Z

Queued a00e9c1413f84459311a0b7c905264d28c199c08 with parent c5ecc15, future comparison URL.

rust-timer · 2021-12-16T01:05:30Z

Finished benchmarking commit (a00e9c1413f84459311a0b7c905264d28c199c08): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

Moderate regression in instruction counts (up to 1.5% on full builds of wg-grammar)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

compiler/rustc_infer/src/traits/mod.rs

estebank · 2021-12-17T18:33:32Z

compiler/rustc_infer/src/traits/mod.rs

+        let expected_new = iter.len();
+        let combined_size = initial_size + expected_new;
+
+        if combined_size <= 16 || combined_size <= current_capacity {


Could using SmallVec be an alternative to making these checks yourself?

The checks here are to determine when to skip deduplication work, not directly to reduce memory footprint (although it does indirectly by preventing blowup with duplicate items)

compiler/rustc_infer/src/traits/mod.rs

estebank · 2021-12-17T18:38:12Z

compiler/rustc_infer/src/traits/mod.rs

+                };
+            };
+
+            self.obligations.retain(|obligation| !is_duplicate(obligation));


I know that we don't dedup earlier because of cache effects (a perf optimization), but doesn't that mean that you always incur the O(n) cost of deduplicating the obligations caught here? Can we check what the perf implications would be of having state in the struct for "self.obligations is already deduped"?

but doesn't that mean that you always incur the O(n) cost of deduplicating the obligations caught here?

Well, not always the vec is grown, no, that would result in O(n²) time. It's amortized O(n) because we only do it when the vec would grow.

Can we check what the perf implications would be of having state in the struct for "self.obligations is already deduped"?

Currently the dedup struct is only constructed temporarily in method scope, it lives much shorter than the vec, so it can't remember whether the vec has already been deduped. It would require larger changes to plumb the dedup state through all method calls that pass the &mut Vec<Obligations> around. That's what I meant with the following in the PR description

or we need to start plumbing the deduplication or a sorted set all the way through the APIs that currently use Vec.

estebank · 2021-12-17T18:40:28Z

compiler/rustc_infer/src/traits/mod.rs

+        ObligationsDedup { obligations: vec }
+    }
+
+    pub fn extend<'b>(&mut self, iter: impl ExactSizeIterator<Item = Cow<'b, Obligation<'tcx, T>>>)


I'm somewhat confused. Why does Item need to be a Cow? I'm not seeing where borrows make sense, but I might be missing something ^_^

Wouldn't passing a &[Obligation<'tcx, T>] give us maybe better perf? Because doing obligations.into_iter().map(...) in the callers is incurring new allocations in the hot path.

I'm not seeing where borrows make sense, but I might be missing something ^_^

Items from the projection cache are borrowed, items from a fresh project_type + normalization are owned.

Because doing obligations.into_iter().map(...) in the callers is incurring new allocations in the hot path.

Do you mean obligations.extend(result.obligations.into_iter().map(Cow::Owned));? That doesn't allocate new obligations, it just stuffs already owned ones into a Cow::Owned enum variant.

apiraino · 2021-12-23T21:25:38Z

(reminder: after this patch will land on nightly, it's a good candidate for a beta-backport, mentioned on Zulip)

the8472 · 2021-12-23T21:41:44Z

@apiraino I'm unsure whether the PR should be taken as-is or refactored to replace the &mut Vec<Obligation> that is passed through various functions with a &mut ObligationSet or something like it which could be a bit more efficient.

If it's wanted for beta-inclusion I guess the smaller scope makes more sense.

…g isn't needed

jackh726 · 2021-12-30T15:56:12Z

r? @jackh726

I'll take over the review here, but it might take me a couple days due to holidays

jackh726 · 2022-01-30T21:38:43Z

Let's check perf with new benchmark

@bors try @rust-timer queue

rust-timer · 2022-01-30T21:38:45Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-30T21:38:53Z

⌛ Trying commit f29ef49 with merge d644ece99d13e11fc68b6183ddb5b79b4ca2b14b...

bors · 2022-01-30T23:03:26Z

☀️ Try build successful - checks-actions
Build commit: d644ece99d13e11fc68b6183ddb5b79b4ca2b14b (d644ece99d13e11fc68b6183ddb5b79b4ca2b14b)

rust-timer · 2022-01-30T23:03:27Z

Queued d644ece99d13e11fc68b6183ddb5b79b4ca2b14b with parent 8c7f2bf, future comparison URL.

rust-timer · 2022-01-31T04:39:02Z

Finished benchmarking commit (d644ece99d13e11fc68b6183ddb5b79b4ca2b14b): comparison url.

Summary: This benchmark run shows 4 relevant improvements 🎉 but 14 relevant regressions 😿 to instruction counts.

Average relevant regression: 1.5%
Average relevant improvement: -0.5%
Largest improvement in instruction counts: -0.5% on full builds of keccak check
Largest regression in instruction counts: 2.1% on full builds of projection-caching check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

jackh726 · 2022-02-11T17:02:27Z

Does this still fix a perf regression for something not covered by the benchmarks? @the8472

the8472 · 2022-02-11T17:15:28Z

@jackh726 not that I am aware of. There still is some duplication happening in obligations that needs to be deduplicated but there already is the deduplication happening later in impl_or_trait_obligations, so this change would mostly move it to happen earlier so that the vecs reach a smaller peak size. But there's no reported exponential blowup this would fix.

It does show up as a few potential max-rss reductions in the latest benchmarks. But considering that this isn't consistent with previous perf runs that's possibly noise.

I think there's still some optimization potential here, but it'll require more changes than just this. I'll close it for now.

the8472 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 24, 2021

rust-highfive assigned estebank Nov 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 24, 2021

the8472 mentioned this pull request Nov 24, 2021

Deduplicate obligations in opt_normalize_projection_type #90913

Closed

estebank reviewed Nov 24, 2021

View reviewed changes

compiler/rustc_infer/src/traits/mod.rs Outdated Show resolved Hide resolved

lqd reviewed Nov 24, 2021

View reviewed changes

compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 26, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 26, 2021

the8472 force-pushed the simplify-obligation-cause-hash branch from 34812ba to 441d34f Compare November 29, 2021 21:46

the8472 added the perf-regression-triaged The performance regression has been triaged. label Dec 6, 2021

the8472 mentioned this pull request Dec 6, 2021

Compile-time regression between 1.56.1 and 1.57.0 for deeply nested decorator types #91598

Closed

Aaron1011 mentioned this pull request Dec 8, 2021

Re-introduce concept of projection cache 'completion' #89831

Merged

the8472 force-pushed the simplify-obligation-cause-hash branch from 441d34f to df24a7f Compare December 15, 2021 19:00

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 15, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 16, 2021

estebank reviewed Dec 17, 2021

View reviewed changes

the8472 added 2 commits December 26, 2021 20:28

dedup obligations in opt_normalize_projection_type

4075f43

don't rebuild Vec when dedup might just shrink it enough that resizin…

d8675de

…g isn't needed

the8472 force-pushed the simplify-obligation-cause-hash branch from df24a7f to a83720c Compare December 26, 2021 19:28

remove redundant use_cache check already covered by outer conditional

f29ef49

the8472 force-pushed the simplify-obligation-cause-hash branch from a83720c to f29ef49 Compare December 27, 2021 02:22

rust-highfive assigned jackh726 and unassigned estebank Dec 30, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 30, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2022

jackh726 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 11, 2022

the8472 closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate obligations in `opt_normalize_projection_type` #91186

Deduplicate obligations in `opt_normalize_projection_type` #91186

the8472 commented Nov 24, 2021

rust-highfive commented Nov 24, 2021

Aaron1011 commented Nov 26, 2021

rust-timer commented Nov 26, 2021

bors commented Nov 26, 2021

bors commented Nov 26, 2021

rust-timer commented Nov 26, 2021

rust-timer commented Nov 26, 2021

the8472 commented Nov 26, 2021

the8472 commented Dec 7, 2021

bors commented Dec 15, 2021

the8472 commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

rust-timer commented Dec 15, 2021

rust-timer commented Dec 16, 2021

estebank Dec 17, 2021

the8472 Dec 21, 2021 •

edited

Loading

estebank Dec 17, 2021

the8472 Dec 21, 2021

estebank Dec 17, 2021

the8472 Dec 21, 2021

apiraino commented Dec 23, 2021

the8472 commented Dec 23, 2021

jackh726 commented Dec 30, 2021

jackh726 commented Jan 30, 2022

rust-timer commented Jan 30, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

rust-timer commented Jan 30, 2022

rust-timer commented Jan 31, 2022

jackh726 commented Feb 11, 2022

the8472 commented Feb 11, 2022

Deduplicate obligations in opt_normalize_projection_type #91186

Deduplicate obligations in opt_normalize_projection_type #91186

Conversation

the8472 commented Nov 24, 2021

rust-highfive commented Nov 24, 2021

Aaron1011 commented Nov 26, 2021

rust-timer commented Nov 26, 2021

bors commented Nov 26, 2021

bors commented Nov 26, 2021

rust-timer commented Nov 26, 2021

rust-timer commented Nov 26, 2021

the8472 commented Nov 26, 2021

the8472 commented Dec 7, 2021

bors commented Dec 15, 2021

the8472 commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

rust-timer commented Dec 15, 2021

rust-timer commented Dec 16, 2021

estebank Dec 17, 2021

Choose a reason for hiding this comment

the8472 Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

estebank Dec 17, 2021

Choose a reason for hiding this comment

the8472 Dec 21, 2021

Choose a reason for hiding this comment

estebank Dec 17, 2021

Choose a reason for hiding this comment

the8472 Dec 21, 2021

Choose a reason for hiding this comment

apiraino commented Dec 23, 2021

the8472 commented Dec 23, 2021

jackh726 commented Dec 30, 2021

jackh726 commented Jan 30, 2022

rust-timer commented Jan 30, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

rust-timer commented Jan 30, 2022

rust-timer commented Jan 31, 2022

jackh726 commented Feb 11, 2022

the8472 commented Feb 11, 2022

Deduplicate obligations in `opt_normalize_projection_type` #91186

Deduplicate obligations in `opt_normalize_projection_type` #91186

the8472 Dec 21, 2021 •

edited

Loading