Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a621ec3
RegionInferenceContext: remove Rc from rev_scc_graph field
klensy Apr 24, 2023
597c651
Arc -> Lrc
klensy Apr 28, 2023
0726636
Lrc -> Rc
klensy Apr 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions compiler/rustc_borrowck/src/region_infer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub struct RegionInferenceContext<'tcx> {
/// Reverse of the SCC constraint graph -- i.e., an edge `A -> B` exists if
/// `B: A`. This is used to compute the universal regions that are required
/// to outlive a given SCC. Computed lazily.
rev_scc_graph: Option<Rc<ReverseSccGraph>>,
rev_scc_graph: Option<ReverseSccGraph>,

/// The "R0 member of [R1..Rn]" constraints, indexed by SCC.
member_constraints: Rc<MemberConstraintSet<'tcx, ConstraintSccIndex>>,
Expand Down Expand Up @@ -813,9 +813,9 @@ impl<'tcx> RegionInferenceContext<'tcx> {
// free region that must outlive the member region `R0` (`UB:
// R0`). Therefore, we need only keep an option `O` if `UB: O`
// for all UB.
let rev_scc_graph = self.reverse_scc_graph();
self.compute_reverse_scc_graph();
let universal_region_relations = &self.universal_region_relations;
for ub in rev_scc_graph.upper_bounds(scc) {
for ub in self.rev_scc_graph.as_ref().unwrap().upper_bounds(scc) {
debug!(?ub);
choice_regions.retain(|&o_r| universal_region_relations.outlives(ub, o_r));
}
Expand Down
13 changes: 5 additions & 8 deletions compiler/rustc_borrowck/src/region_infer/reverse_sccs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use rustc_data_structures::graph::vec_graph::VecGraph;
use rustc_data_structures::graph::WithSuccessors;
use rustc_middle::ty::RegionVid;
use std::ops::Range;
use std::rc::Rc;

pub(crate) struct ReverseSccGraph {
graph: VecGraph<ConstraintSccIndex>,
Expand Down Expand Up @@ -40,10 +39,10 @@ impl ReverseSccGraph {
}

impl RegionInferenceContext<'_> {
/// Compute and return the reverse SCC-based constraint graph (lazily).
pub(super) fn reverse_scc_graph(&mut self) -> Rc<ReverseSccGraph> {
if let Some(g) = &self.rev_scc_graph {
return g.clone();
/// Compute the reverse SCC-based constraint graph (lazily).
pub(super) fn compute_reverse_scc_graph(&mut self) {
if self.rev_scc_graph.is_some() {
return;
}

let graph = self.constraint_sccs.reverse();
Expand All @@ -63,8 +62,6 @@ impl RegionInferenceContext<'_> {
start += group_size;
}

let rev_graph = Rc::new(ReverseSccGraph { graph, scc_regions, universal_regions });
self.rev_scc_graph = Some(rev_graph.clone());
rev_graph
self.rev_scc_graph = Some(ReverseSccGraph { graph, scc_regions, universal_regions });
}
}
23 changes: 12 additions & 11 deletions compiler/rustc_expand/src/mbe/macro_parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ use rustc_span::Span;
use std::borrow::Cow;
use std::collections::hash_map::Entry::{Occupied, Vacant};
use std::fmt::Display;
use std::rc::Rc;

/// A unit within a matcher that a `MatcherPos` can refer to. Similar to (and derived from)
/// `mbe::TokenTree`, but designed specifically for fast and easy traversal during matching.
Expand Down Expand Up @@ -257,10 +258,10 @@ struct MatcherPos {
/// against the relevant metavar by the black box parser. An element will be a `MatchedSeq` if
/// the corresponding metavar decl is within a sequence.
///
/// It is critical to performance that this is an `Lrc`, because it gets cloned frequently when
/// It is critical to performance that this is an `Rc`, because it gets cloned frequently when
/// processing sequences. Mostly for sequence-ending possibilities that must be tried but end
/// up failing.
matches: Lrc<Vec<NamedMatch>>,
matches: Rc<Vec<NamedMatch>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@klensy klensy May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we can leave those nits for the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>>`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. Clone requiring Sized was a mistake :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// This type is used a lot. Make sure it doesn't unintentionally get bigger.
Expand All @@ -272,7 +273,7 @@ impl MatcherPos {
/// and both are hot enough to be always worth inlining.
#[inline(always)]
fn push_match(&mut self, metavar_idx: usize, seq_depth: usize, m: NamedMatch) {
let matches = Lrc::make_mut(&mut self.matches);
let matches = Rc::make_mut(&mut self.matches);
match seq_depth {
0 => {
// We are not within a sequence. Just append `m`.
Expand Down Expand Up @@ -427,7 +428,7 @@ pub struct TtParser {

/// Pre-allocate an empty match array, so it can be cloned cheaply for macros with many rules
/// that have no metavars.
empty_matches: Lrc<Vec<NamedMatch>>,
empty_matches: Rc<Vec<NamedMatch>>,
}

impl TtParser {
Expand All @@ -437,7 +438,7 @@ impl TtParser {
cur_mps: vec![],
next_mps: vec![],
bb_mps: vec![],
empty_matches: Lrc::new(vec![]),
empty_matches: Rc::new(vec![]),
}
}

Expand Down Expand Up @@ -507,7 +508,7 @@ impl TtParser {
// Try zero matches of this sequence, by skipping over it.
self.cur_mps.push(MatcherPos {
idx: idx_first_after,
matches: Lrc::clone(&mp.matches),
matches: Rc::clone(&mp.matches),
});
}

Expand All @@ -521,7 +522,7 @@ impl TtParser {
// processed next time around the loop.
let ending_mp = MatcherPos {
idx: mp.idx + 1, // +1 skips the Kleene op
matches: Lrc::clone(&mp.matches),
matches: Rc::clone(&mp.matches),
};
self.cur_mps.push(ending_mp);

Expand All @@ -537,7 +538,7 @@ impl TtParser {
// will fail quietly when it is processed next time around the loop.
let ending_mp = MatcherPos {
idx: mp.idx + 2, // +2 skips the separator and the Kleene op
matches: Lrc::clone(&mp.matches),
matches: Rc::clone(&mp.matches),
};
self.cur_mps.push(ending_mp);

Expand Down Expand Up @@ -587,9 +588,9 @@ impl TtParser {
if *token == token::Eof {
Some(match eof_mps {
EofMatcherPositions::One(mut eof_mp) => {
// Need to take ownership of the matches from within the `Lrc`.
Lrc::make_mut(&mut eof_mp.matches);
let matches = Lrc::try_unwrap(eof_mp.matches).unwrap().into_iter();
// Need to take ownership of the matches from within the `Rc`.
Rc::make_mut(&mut eof_mp.matches);
let matches = Rc::try_unwrap(eof_mp.matches).unwrap().into_iter();
Comment on lines +592 to +593
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

self.nameize(matcher, matches)
}
EofMatcherPositions::Multiple => {
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_passes/src/debugger_visualizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@

use hir::CRATE_HIR_ID;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::sync::Lrc;
use rustc_expand::base::resolve_path;
use rustc_hir as hir;
use rustc_hir::HirId;
use rustc_middle::ty::TyCtxt;
use rustc_middle::{query::LocalCrate, ty::query::Providers};
use rustc_span::{sym, DebuggerVisualizerFile, DebuggerVisualizerType};

use std::sync::Arc;

use crate::errors::DebugVisualizerUnreadable;

fn check_for_debugger_visualizer(
Expand Down Expand Up @@ -52,7 +51,7 @@ fn check_for_debugger_visualizer(
match std::fs::read(&file) {
Ok(contents) => {
debugger_visualizers
.insert(DebuggerVisualizerFile::new(Arc::from(contents), visualizer_type));
.insert(DebuggerVisualizerFile::new(Lrc::from(contents), visualizer_type));
}
Err(error) => {
tcx.sess.emit_err(DebugVisualizerUnreadable {
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_span/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ use std::hash::Hash;
use std::ops::{Add, Range, Sub};
use std::path::{Path, PathBuf};
use std::str::FromStr;
use std::sync::Arc;

use md5::Digest;
use md5::Md5;
Expand Down Expand Up @@ -1269,13 +1268,13 @@ pub enum DebuggerVisualizerType {
#[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, Encodable, Decodable)]
pub struct DebuggerVisualizerFile {
/// The complete debugger visualizer source.
pub src: Arc<[u8]>,
pub src: Lrc<[u8]>,
/// Indicates which visualizer type this targets.
pub visualizer_type: DebuggerVisualizerType,
}

impl DebuggerVisualizerFile {
pub fn new(src: Arc<[u8]>, visualizer_type: DebuggerVisualizerType) -> Self {
pub fn new(src: Lrc<[u8]>, visualizer_type: DebuggerVisualizerType) -> Self {
DebuggerVisualizerFile { src, visualizer_type }
}
}
Expand Down