[perf] separate impls for refs to simplifiable types #106155

lqd · 2022-12-26T15:36:25Z

As discussed on zulip, some of the coherence checks on the window crates are hot on RefSimplifiedTypes, I believe because the crate contains a lot of types with From conversions between them (emulating COM hierarchies IIUC), including references to these types.

This PR separates impls for references to simplifiable types from the current non-blanket impls, for faster lookups.

Opening as draft for feedback purposes, especially as to what kind of tests @lcnr would like to see in this PR.

lqd · 2022-12-26T15:37:15Z

@bors try @rust-timer queue

bors · 2022-12-26T15:37:23Z

⌛ Trying commit 89df198d30df99b9bc5c6738380f0b7b3759a1af with merge 8185d21406dfb1106547fc5cb973f6dffdc2b574...

bors · 2022-12-26T18:17:43Z

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

rust-timer · 2022-12-26T19:35:59Z

Finished benchmarking commit (8185d21406dfb1106547fc5cb973f6dffdc2b574): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

lqd · 2022-12-26T22:44:53Z

I'm not sure this is complete just yet or if it's still missing some important changes, so I'll leave it as draft for now, and I'd also like to add some tests.

But since this topic showed up in the discussion to reduce hotspots for the atypical windows-rs project, I've tried a cargo check --all-features for the windows crate, comparing the try build 8185d21406dfb1106547fc5cb973f6dffdc2b574 to its parent 731e0bf721c1ec2c7843547e86b6157b40a437d1:

for a non-incremental check, the try build was around 9% faster: 330s vs 360s.
for an incremental check, the try build was around 11% faster: 486s vs 545s.

As shown in the perf run above, these patterns are quite uncommon in our benchmark suite, and I believe in the wider ecosystem, but they do stress coherence, and could be present in other similar big crates bindings to existing type hierarchies (e.g. to other OS APIs, maybe UI toolkits with OOP flavors, etc).

With that in mind, r? @lcnr for when they come back from vacation.

lcnr · 2023-01-09T11:08:03Z

compiler/rustc_middle/src/ty/trait_def.rs

+                if let Some(impls) = impls.impls_for_ref_x.get(&simplified_ref_ty) {
+                    return impls.iter().copied();
+                }
+            }


missing the impls_for_ref_x impls if you have &T. Doesn't matter with how this method is used but we should still be careful for it to be correct.

lcnr

a few nits, after that

r=me

compiler/rustc_middle/src/ty/trait_def.rs

lcnr · 2023-01-19T10:52:03Z

ah for tests:

probably:

&_ correctly detecting impls for &u32.

coherence: &T overlapping with &u32

impls with &u32 specializing an impl of &T and correctly using associated item definitions of the &T impl in the &u32 impl.

That should cover all the edge cases I can think of rn

Separate impls for references to simplifiable types to allow for faster lookups: the set of possibly matching impls is smaller than if we stored them all as `SimplifiedType::RefSimplifiedType`.

handle `impls_for_ref_x` and reduce duplication when iterating over all impls. also: use it in `for_each_relevant_impl_treating_projections`

for users of the API, we return impls from `non_blanket_impls` and `impls_for_ref_x` as if they weren't separated.

lqd · 2023-05-31T21:16:41Z

(rebased to discuss with lcnr, but the PR is still waiting on me)

@bors try

bors · 2023-05-31T21:16:50Z

⌛ Trying commit 030f9bb with merge cd2704ff205c1cca87105183ec3fc80182b8f5bb...

bors · 2023-05-31T23:27:13Z

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

lqd · 2023-06-01T12:53:50Z

Sanity check on windows-rs with the try build cd2704ff205c1cca87105183ec3fc80182b8f5bb and its parent 871b5952023139738f72eba235063575062bc2e9, on full and incr-full scenarios:

Benchmark 1: CARGO_INCREMENTAL=0 cargo +winrs-try-parent check --all-features
  Time (mean ± σ):     336.616 s ±  0.582 s    [User: 327.807 s, System: 12.608 s]
  Range (min … max):   335.954 s … 337.044 s    3 runs

Benchmark 2: CARGO_INCREMENTAL=0 cargo +winrs-try check --all-features
  Time (mean ± σ):     309.976 s ±  1.063 s    [User: 301.125 s, System: 12.671 s]
  Range (min … max):   309.017 s … 311.119 s    3 runs

Summary
  'CARGO_INCREMENTAL=0 cargo +winrs-try check --all-features' ran
    1.09 ± 0.00 times faster than 'CARGO_INCREMENTAL=0 cargo +winrs-try-parent check --all-features'

Benchmark 1: cargo +winrs-try-parent check --all-features
  Time (mean ± σ):     533.431 s ±  0.708 s    [User: 511.599 s, System: 25.910 s]
  Range (min … max):   532.816 s … 534.205 s    3 runs

Benchmark 2: cargo +winrs-try check --all-features
  Time (mean ± σ):     477.190 s ±  0.913 s    [User: 456.594 s, System: 24.689 s]
  Range (min … max):   476.411 s … 478.195 s    3 runs

Summary
  'cargo +winrs-try check --all-features' ran
    1.12 ± 0.00 times faster than 'cargo +winrs-try-parent check --all-features'

lqd · 2023-08-08T13:26:21Z

Coherence checks are on their way of being switched from the current solver to the next trait solver, via -Ztrait-solver=next-coherence. It performs the same in the rustc-perf benchmarks, and on windows-rs.

However, this PR while being very beneficial to e.g. windows-rs, is very specific to the old solver architecture, and cannot be applied to the new solver. After discussing with @lcnr, we decided to close this PR because it will become obsolete as soon as the new solver is used for coherence, and landing prior to that, only for a short period, wouldn't be particularly useful.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 26, 2022

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

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

lqd force-pushed the simplified-ref branch from 89df198 to e63f8c9 Compare December 26, 2022 22:03

rustbot assigned lcnr Dec 26, 2022

lcnr reviewed Jan 9, 2023

View reviewed changes

compiler/rustc_middle/src/ty/trait_def.rs Show resolved Hide resolved

lcnr 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 Jan 19, 2023

This comment was marked as outdated.

Sign in to view

lqd added 3 commits May 31, 2023 19:33

separate impls for refs to simplifiable types

a9d7810

Separate impls for references to simplifiable types to allow for faster lookups: the set of possibly matching impls is smaller than if we stored them all as `SimplifiedType::RefSimplifiedType`.

clean-up for_each_impl to use all_impls

f2b58f3

handle `impls_for_ref_x` and reduce duplication when iterating over all impls. also: use it in `for_each_relevant_impl_treating_projections`

return refs to simplifiable types in non_blanket_impls_for_ty

030f9bb

for users of the API, we return impls from `non_blanket_impls` and `impls_for_ref_x` as if they weren't separated.

lqd force-pushed the simplified-ref branch from e63f8c9 to 030f9bb Compare May 31, 2023 20:41

lqd closed this Aug 8, 2023

lqd deleted the simplified-ref branch August 8, 2023 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[perf] separate impls for refs to simplifiable types #106155

[perf] separate impls for refs to simplifiable types #106155

lqd commented Dec 26, 2022 •

edited

Loading

lqd commented Dec 26, 2022

This comment has been minimized.

bors commented Dec 26, 2022

bors commented Dec 26, 2022

This comment has been minimized.

rust-timer commented Dec 26, 2022

lqd commented Dec 26, 2022 •

edited

Loading

lcnr Jan 9, 2023

lcnr left a comment

lcnr commented Jan 19, 2023

This comment was marked as outdated.

lqd commented May 31, 2023

bors commented May 31, 2023

bors commented May 31, 2023

lqd commented Jun 1, 2023

lqd commented Aug 8, 2023

[perf] separate impls for refs to simplifiable types #106155

[perf] separate impls for refs to simplifiable types #106155

Conversation

lqd commented Dec 26, 2022 • edited Loading

lqd commented Dec 26, 2022

This comment has been minimized.

bors commented Dec 26, 2022

bors commented Dec 26, 2022

This comment has been minimized.

rust-timer commented Dec 26, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

lqd commented Dec 26, 2022 • edited Loading

lcnr Jan 9, 2023

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Jan 19, 2023

This comment was marked as outdated.

lqd commented May 31, 2023

bors commented May 31, 2023

bors commented May 31, 2023

lqd commented Jun 1, 2023

lqd commented Aug 8, 2023

lqd commented Dec 26, 2022 •

edited

Loading

lqd commented Dec 26, 2022 •

edited

Loading