try to downgrade Arc -> Lrc -> Rc -> no-Rc in few places #111014

klensy · 2023-04-30T10:34:23Z

Expecting this be not slower on non-parallel compiler and probably faster on parallel (checked that this PR builds on it).

rustbot · 2023-04-30T10:34:29Z

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

WaffleLapkin

a couple of nits

WaffleLapkin · 2023-05-01T09:49:21Z

compiler/rustc_expand/src/mbe/macro_parser.rs

    /// processing sequences. Mostly for sequence-ending possibilities that must be tried but end
    /// up failing.
-    matches: Lrc<Vec<NamedMatch>>,
+    matches: Rc<Vec<NamedMatch>>,


could this be Rc<[NamedMatch]>? would it be better?

Maybe. I've tried to only change rc stuff. This thing noted as hot, so changing vec<->slice can potentially shadow other changes. @nnethercote changed this last time, maybe he already tried that and found, that there no gains?

Okay, we can leave those nits for the future.

I just tried it, there are tons of problems.

error[E0277]: the trait bound `[NamedMatch]: Clone` is not satisfied --> compiler/rustc_expand/src/mbe/macro_parser.rs:275:37 | 275 | let matches = Lrc::make_mut(&mut self.matches); | ------------- ^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `[NamedMatch]` | | | required by a bound introduced by this call | = help: the trait `Clone` is implemented for `[T; N]` note: required by a bound in `Lrc::<T>::make_mut` --> /home/njn/dev/rust1/library/alloc/src/rc.rs:1191:9 | 1191 | impl<T: Clone> Rc<T> { | ^^^^^ required by this bound in `Rc::<T>::make_mut` error[E0599]: no method named `push` found for mutable reference `&mut [NamedMatch]` in the current scope --> compiler/rustc_expand/src/mbe/macro_parser.rs:280:25 | 280 | matches.push(m); | ^^^^ method not found in `&mut [NamedMatch]` error[E0277]: the trait bound `[NamedMatch]: Clone` is not satisfied --> compiler/rustc_expand/src/mbe/macro_parser.rs:591:35 | 591 | Lrc::make_mut(&mut eof_mp.matches); | ------------- ^^^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `[NamedMatch]` | | | required by a bound introduced by this call | = help: the trait `Clone` is implemented for `[T; N]` note: required by a bound in `Lrc::<T>::make_mut` --> /home/njn/dev/rust1/library/alloc/src/rc.rs:1191:9 | 1191 | impl<T: Clone> Rc<T> { | ^^^^^ required by this bound in `Rc::<T>::make_mut` error[E0277]: the size for values of type `[NamedMatch]` cannot be known at compilation time --> compiler/rustc_expand/src/mbe/macro_parser.rs:592:51 | 592 | let matches = Lrc::try_unwrap(eof_mp.matches).unwrap().into_iter(); | --------------- ^^^^^^^^^^^^^^ doesn't have a size known at compile-time | | | required by a bound introduced by this call | = help: the trait `Sized` is not implemented for `[NamedMatch]` note: required by a bound in `Lrc::<T>::try_unwrap` --> /home/njn/dev/rust1/library/alloc/src/rc.rs:360:6 | 360 | impl<T> Rc<T> { | ^ required by this bound in `Rc::<T>::try_unwrap` error[E0599]: the method `unwrap` exists for enum `Result<[NamedMatch], Rc<[NamedMatch]>>`, but its trait bounds were not satisfied --> compiler/rustc_expand/src/mbe/macro_parser.rs:592:67 | 592 | let matches = Lrc::try_unwrap(eof_mp.matches).unwrap().into_iter(); | ^^^^^^ method cannot be called on `Result<[NamedMatch], Rc<[NamedMatch]>>` due to unsatisfied trait bounds | = note: the following trait bounds were not satisfied: `[NamedMatch]: Sized` error[E0277]: the size for values of type `[NamedMatch]` cannot be known at compilation time --> compiler/rustc_expand/src/mbe/macro_parser.rs:592:35 | 592 | let matches = Lrc::try_unwrap(eof_mp.matches).unwrap().into_iter(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time | = help: the trait `Sized` is not implemented for `[NamedMatch]` note: required by a bound in `Result` --> /home/njn/dev/rust1/library/core/src/result.rs:502:17 | 502 | pub enum Result<T, E> { | ^ required by this bound in `Result` error[E0308]: mismatched types --> compiler/rustc_expand/src/mbe/macro_parser.rs:625:57 | 625 | self.cur_mps.push(MatcherPos { idx: 0, matches: self.empty_matches.clone() }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Rc<[NamedMatch]>`, found `Rc<Vec<NamedMatch>>` | = note: expected struct `Lrc<[NamedMatch]>` found struct `Lrc<Vec<NamedMatch>>`

Ugh. Clone requiring Sized was a mistake :(

Perhaps, but that's not the only problem here.

WaffleLapkin · 2023-05-01T09:52:22Z

compiler/rustc_expand/src/mbe/macro_parser.rs

+                    Rc::make_mut(&mut eof_mp.matches);
+                    let matches = Rc::try_unwrap(eof_mp.matches).unwrap().into_iter();


Would it make sense to just

Suggested change

Rc::make_mut(&mut eof_mp.matches);

let matches = Rc::try_unwrap(eof_mp.matches).unwrap().into_iter();

let matches = Rc::make_mut(&mut eof_mp.matches).take().into_iter();

or even

Suggested change

Rc::make_mut(&mut eof_mp.matches);

let matches = Rc::try_unwrap(eof_mp.matches).unwrap().into_iter();

eof_mp.matches.iter().cloned()

WaffleLapkin · 2023-05-04T18:32:33Z

@bors r+ rollup=never

bors · 2023-05-04T18:32:35Z

📌 Commit 0726636 has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-05-04T20:49:26Z

⌛ Testing commit 0726636 with merge 74c4821...

nnethercote · 2023-05-04T21:40:22Z

@klensy: how did you find these? Did you just spot them manually, or did you have some kind of process for finding them?

klensy · 2023-05-04T21:55:08Z

@klensy: how did you find these? Did you just spot them manually, or did you have some kind of process for finding them?

Manual process: switched build to parallel = true, walked over Arc - Lrc - Rc (there not so many places with them), an tried to use next rc kind, if it looks easy and don't lead to rewriting more than i want. There more things probably can be rewritten, for example in RegionInferenceContext.

bors · 2023-05-04T23:40:51Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 74c4821 to master...

rust-timer · 2023-05-05T00:57:25Z

Finished benchmarking commit (74c4821): 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)	4.2%	[3.7%, 4.6%]	2
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	4.2%	[3.7%, 4.6%]	2

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)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 657.762s -> 654.9s (-0.44%)

klensy added 3 commits April 30, 2023 12:18

RegionInferenceContext: remove Rc from rev_scc_graph field

a621ec3

Arc -> Lrc

597c651

Lrc -> Rc

0726636

rustbot assigned WaffleLapkin Apr 30, 2023

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 Apr 30, 2023

WaffleLapkin reviewed May 1, 2023

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 May 4, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label May 4, 2023

bors merged commit 74c4821 into rust-lang:master May 4, 2023

rustbot added this to the 1.71.0 milestone May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

try to downgrade Arc -> Lrc -> Rc -> no-Rc in few places #111014

try to downgrade Arc -> Lrc -> Rc -> no-Rc in few places #111014

klensy commented Apr 30, 2023

rustbot commented Apr 30, 2023

WaffleLapkin left a comment

WaffleLapkin May 1, 2023

klensy May 4, 2023 •

edited

Loading

WaffleLapkin May 4, 2023

nnethercote May 4, 2023

WaffleLapkin May 5, 2023

nnethercote May 5, 2023

WaffleLapkin May 1, 2023

WaffleLapkin commented May 4, 2023

bors commented May 4, 2023

bors commented May 4, 2023

nnethercote commented May 4, 2023

klensy commented May 4, 2023

bors commented May 4, 2023

rust-timer commented May 5, 2023

		Rc::make_mut(&mut eof_mp.matches);
		let matches = Rc::try_unwrap(eof_mp.matches).unwrap().into_iter();

	Rc::make_mut(&mut eof_mp.matches);
	let matches = Rc::try_unwrap(eof_mp.matches).unwrap().into_iter();
	let matches = Rc::make_mut(&mut eof_mp.matches).take().into_iter();

	Rc::make_mut(&mut eof_mp.matches);
	let matches = Rc::try_unwrap(eof_mp.matches).unwrap().into_iter();
	eof_mp.matches.iter().cloned()

try to downgrade Arc -> Lrc -> Rc -> no-Rc in few places #111014

try to downgrade Arc -> Lrc -> Rc -> no-Rc in few places #111014

Conversation

klensy commented Apr 30, 2023

rustbot commented Apr 30, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klensy May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaffleLapkin commented May 4, 2023

bors commented May 4, 2023

bors commented May 4, 2023

nnethercote commented May 4, 2023

klensy commented May 4, 2023

bors commented May 4, 2023

rust-timer commented May 5, 2023

Overall result: no relevant changes - no action needed

klensy May 4, 2023 •

edited

Loading