get rid of `RefCell` in `TransitiveRelation` #99702

SparrowLii · 2022-07-25T02:03:17Z

This is one of the jobs in Pending refactorings in #48685. The parallel-compiler's work has been suspended for quite some time, but I think I can pick it up gradually. I think this PR should be a start.

Regarding the refactoring of TransitiveRelation, @nikomatsakis has proposed two(three?) schemes. In order to satisfy both compilation efficiency and robustness, I think adding the freeze method may be the best solution, although it requires relatively more code changes.

rust-highfive · 2022-07-25T02:03:20Z

r? @petrochenkov

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

petrochenkov · 2022-07-25T14:36:00Z

I'm not familiar with this area.
r? rust-lang/compiler

compiler/rustc_infer/src/infer/outlives/env.rs

oli-obk · 2022-07-28T09:14:53Z

please rebase so we can run a perf test on this.

cc @lcnr this may conflict with your other refactorings

lcnr · 2022-07-28T11:04:12Z

I intend to add implied bounds to the param env so OutlivesEnvironment::with_implied_bounds will should be unnecessary in a few weeks/months. Can't think of anything else I am working on which will conflict with this in the near future.

SparrowLii · 2022-07-29T07:14:43Z

I add implied_bounds_tys method to outlives_bounds::InferCtxtExt to collect all the implied_bounds, then add with_bounds to OutlivesEnvironment to build at once. In this case we don't need regionck mod in typeck anymore.

@oli-obk I rebased the PR so we can do a perf run now.

lqd · 2022-07-29T07:24:31Z

@bors try @rust-timer queue

rust-timer · 2022-07-29T07:24:32Z

Awaiting bors try build completion.

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

bors · 2022-07-29T07:24:39Z

⌛ Trying commit 53dc99a465b159eb13c3ac2ecd2924dfe65beab3 with merge d4e9c7f1d17ad93a14e06c762e2177954744128a...

bors · 2022-07-29T09:19:05Z

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

rust-timer · 2022-07-29T09:19:07Z

Queued d4e9c7f1d17ad93a14e06c762e2177954744128a with parent ea6ab1b, future comparison URL.

rust-timer · 2022-07-29T10:55:13Z

Finished benchmarking commit (d4e9c7f1d17ad93a14e06c762e2177954744128a): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.3%	0.3%	2
Regressions 😿 (secondary)	0.2%	0.3%	3
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.2%	-0.2%	1
All 😿🎉 (primary)	0.3%	0.3%	2

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.6%	2.6%	2
Improvements 🎉 (primary)	-2.5%	-2.5%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.5%	-2.5%	1

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	10.4%	10.4%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	10.4%	10.4%	1

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 may lead 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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

oli-obk · 2022-07-29T12:47:03Z

Small perf changes:

--------------------------------------------------------------------------------
Ir        
--------------------------------------------------------------------------------
1,636,568  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir          file:function
--------------------------------------------------------------------------------
 5,438,542  ???:rustc_borrowck::type_check::free_region_relations::create
-3,075,341  ???:<rustc_borrowck::region_infer::RegionInferenceContext>::solve
 1,352,946  ???:<rustc_infer::infer::outlives::env::OutlivesEnvironment>::with_bounds::<alloc::boxed::Box<dyn core::iter::traits::iterator::Iterator<Item = rustc_middle::traits::query::OutlivesBound>>>
-1,081,605  ???:<alloc::rc::Rc<rustc_middle::traits::ObligationCauseCode> as core::ops::drop::Drop>::drop
 1,072,968  ???:<rustc_mir_dataflow::impls::MaybeInitializedPlaces as rustc_mir_dataflow::framework::Analysis>::into_engine
-1,068,823  ???:<rustc_mir_dataflow::framework::engine::Engine<rustc_mir_dataflow::impls::MaybeInitializedPlaces>>::new_gen_kill
 1,037,740  ???:<rustc_infer::infer::InferCtxtBuilder>::enter::<core::result::Result<alloc::vec::Vec<rustc_middle::ty::Predicate>, rustc_errors::ErrorGuaranteed>, rustc_trait_selection::traits::do_normalize_predicates::{closure
   887,900  ???:<rustc_mir_dataflow::impls::MaybeUninitializedPlaces as rustc_mir_dataflow::framework::Analysis>::into_engine
   881,271  ???:<rustc_mir_dataflow::impls::EverInitializedPlaces as rustc_mir_dataflow::framework::Analysis>::into_engine
   870,077  ???:<hashbrown::raw::RawTable<(rustc_span::def_id::DefId, (core::option::Option<rustc_middle::ty::Ty>, rustc_query_system::dep_graph::graph::DepNodeIndex))>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_span::def_id::DefId, rustc_span::def_id::DefId, (core::option::Option<rustc_middle::ty::Ty>, rustc_query_system::dep_graph::graph::DepNodeIndex), core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
  -869,973  ???:<hashbrown::raw::RawTable<(rustc_span::def_id::DefId, (rustc_middle::ty::adt::AdtDef, rustc_query_system::dep_graph::graph::DepNodeIndex))>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_span::def_id::DefId, rustc_span::def_id::DefId, (rustc_middle::ty::adt::AdtDef, rustc_query_system::dep_graph::graph::DepNodeIndex), core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
  -868,377  ???:<rustc_mir_dataflow::framework::engine::Engine<rustc_mir_dataflow::impls::EverInitializedPlaces>>::new_gen_kill
  -857,624  ???:<rustc_mir_dataflow::framework::engine::Engine<rustc_mir_dataflow::impls::MaybeUninitializedPlaces>>::new_gen_kill
  -708,799  ???:<rustc_infer::infer::outlives::env::OutlivesEnvironment as rustc_typeck::check::regionck::OutlivesEnvironmentExt>::add_implied_bounds
  -700,204  ???:<rustc_infer::infer::outlives::env::OutlivesEnvironment>::new
   662,528  ???:<rustc_infer::infer::outlives::env::OutlivesEnvironment>::builder

mostly inlining changes (new_gen_kill -> into_engine)

compiler/rustc_typeck/src/outlives/outlives_bounds.rs

SparrowLii · 2022-07-30T02:01:51Z

mostly inlining changes (new_gen_kill -> into_engine)

I add inline on the into_engine func, hoping this can help

rust-timer · 2022-08-01T03:02:43Z

Awaiting bors try build completion.

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

bors · 2022-08-01T03:02:52Z

⌛ Trying commit d16796047523a6a17de08dcb739604f402526fb8 with merge e4ad27f1e1bd50aac91b50bd072257da970f393b...

bors · 2022-08-01T04:41:42Z

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

rust-timer · 2022-08-01T04:41:44Z

Queued e4ad27f1e1bd50aac91b50bd072257da970f393b with parent 34805f3, future comparison URL.

rust-timer · 2022-08-01T06:32:31Z

Finished benchmarking commit (e4ad27f1e1bd50aac91b50bd072257da970f393b): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.2%	0.2%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

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

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	2.3%	2.3%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.3%	2.3%	1

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

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

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

…o `outlives_bounds::InferCtxtExt`

oli-obk · 2022-08-22T10:47:23Z

@bors r+

bors · 2022-08-22T10:47:25Z

📌 Commit a01ac5a has been approved by oli-obk

It is now in the queue for this repository.

SparrowLii · 2022-08-22T10:48:42Z

Thanks for reviewing!

bors · 2022-08-22T11:39:55Z

⌛ Testing commit a01ac5a with merge a8a33cf...

bors · 2022-08-22T14:26:36Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing a8a33cf to master...

rust-timer · 2022-08-22T16:16:55Z

Finished benchmarking commit (a8a33cf): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.2%, 2.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.7%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.5%	[-5.2%, -3.9%]	2
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rust-highfive assigned petrochenkov Jul 25, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2022

This comment has been minimized.

Sign in to view

SparrowLii force-pushed the transtive_relation branch 2 times, most recently from d9f45cc to 1a665aa Compare July 25, 2022 03:06

rust-highfive assigned oli-obk and unassigned petrochenkov Jul 25, 2022

This comment was marked as resolved.

Sign in to view

oli-obk requested changes Jul 28, 2022

View reviewed changes

compiler/rustc_infer/src/infer/outlives/env.rs Outdated Show resolved Hide resolved

SparrowLii force-pushed the transtive_relation branch from 1a665aa to 53dc99a Compare July 29, 2022 07:10

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

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 29, 2022

oli-obk reviewed Jul 29, 2022

View reviewed changes

compiler/rustc_typeck/src/outlives/outlives_bounds.rs Outdated Show resolved Hide resolved

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

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Aug 1, 2022

SparrowLii added 4 commits August 22, 2022 18:08

get rid of RefCell in TransitiveRelation

5d9e4d0

add with_bounds to OutlivesEnvironment and implied_bounds_tys t…

d037f18

…o `outlives_bounds::InferCtxtExt`

use type alias impl trait in outlives_bounds::InferCtxtExt

d39fefd

re-base and use OutlivesEnvironment::with_bounds

a01ac5a

SparrowLii force-pushed the transtive_relation branch from d167960 to a01ac5a Compare August 22, 2022 10:38

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 22, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2022

bors merged commit a8a33cf into rust-lang:master Aug 22, 2022

rustbot added this to the 1.65.0 milestone Aug 22, 2022

bors mentioned this pull request Aug 22, 2022

Change is_some_and to take by value #98354

Merged

spastorino mentioned this pull request Aug 22, 2022

Coherence negative impls implied bounds #100888

Merged

cjgillot mentioned this pull request Aug 27, 2022

Query Parallelization Tracking Issue #48685

Open

31 tasks

SparrowLii mentioned this pull request Nov 10, 2022

Reboot Parallel Rustc WG Proposal rust-lang/compiler-team#567

Closed

3 tasks

matthiaskrgr mentioned this pull request Jan 7, 2024

ICE: effects, gce: difference in caller_arg caller_abi layout #119701

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get rid of `RefCell` in `TransitiveRelation` #99702

get rid of `RefCell` in `TransitiveRelation` #99702

SparrowLii commented Jul 25, 2022

rust-highfive commented Jul 25, 2022

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Jul 25, 2022

This comment was marked as resolved.

oli-obk commented Jul 28, 2022

lcnr commented Jul 28, 2022

SparrowLii commented Jul 29, 2022

lqd commented Jul 29, 2022

rust-timer commented Jul 29, 2022

bors commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

rust-timer commented Jul 29, 2022

oli-obk commented Jul 29, 2022

SparrowLii commented Jul 30, 2022

rust-timer commented Aug 1, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

rust-timer commented Aug 1, 2022

rust-timer commented Aug 1, 2022

oli-obk commented Aug 22, 2022

bors commented Aug 22, 2022

SparrowLii commented Aug 22, 2022

bors commented Aug 22, 2022

bors commented Aug 22, 2022

rust-timer commented Aug 22, 2022

get rid of RefCell in TransitiveRelation #99702

get rid of RefCell in TransitiveRelation #99702

Conversation

SparrowLii commented Jul 25, 2022

rust-highfive commented Jul 25, 2022

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Jul 25, 2022

This comment was marked as resolved.

oli-obk commented Jul 28, 2022

lcnr commented Jul 28, 2022

SparrowLii commented Jul 29, 2022

lqd commented Jul 29, 2022

rust-timer commented Jul 29, 2022

bors commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

rust-timer commented Jul 29, 2022

Footnotes

oli-obk commented Jul 29, 2022

SparrowLii commented Jul 30, 2022

rust-timer commented Aug 1, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

rust-timer commented Aug 1, 2022

rust-timer commented Aug 1, 2022

Footnotes

oli-obk commented Aug 22, 2022

bors commented Aug 22, 2022

SparrowLii commented Aug 22, 2022

bors commented Aug 22, 2022

bors commented Aug 22, 2022

rust-timer commented Aug 22, 2022

Overall result: no relevant changes - no action needed

Footnotes

get rid of `RefCell` in `TransitiveRelation` #99702

get rid of `RefCell` in `TransitiveRelation` #99702