-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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.
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
Wrap some query results in Lrc
.
#55778
Conversation
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
} else { | ||
tcx.predicates_of(def_id).predicates | ||
tcx.predicates_of(def_id).predicates.clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary clone, can just keep the whole Lrc
in the predicates
variable and change the loop below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and failed to do that. I can't use the Rc
directly, because that would move it. I can't use a reference, because it doesn't live long enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I worked out a different way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason this can't work:
let predicates = if def_id.is_local() {
tcx.explicit_predicates_of(def_id)
} else {
tcx.predicates_of(def_id)
};
Then you do predicates.predicates.iter()
below instead of predicates.into_iter()
.
src/librustc_typeck/collect.rs
Outdated
@@ -1794,7 +1795,7 @@ fn explicit_predicates_of<'a, 'tcx>( | |||
// on a trait we need to add in the supertrait bounds and bounds found on | |||
// associated types. | |||
if let Some((_trait_ref, _)) = is_trait { | |||
predicates.extend(tcx.super_predicates_of(def_id).predicates); | |||
predicates.extend(tcx.super_predicates_of(def_id).predicates.iter().map(|p| *p)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the .map(|p| *p)
here and elsewhere could be replaced by .cloned()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't that cause additional refcounting that the .map(|p| *p)
won't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it does .map(|p| p.clone())
which is the same as .map(|p| *p)
when the latter works because the latter only works when typeof(*p): Copy
and Copy
implies Clone
is just a copy.
@@ -1112,8 +1112,8 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx>+'o { | |||
{ | |||
let tcx = self.tcx(); | |||
|
|||
let bounds = self.get_type_parameter_bounds(span, ty_param_def_id) | |||
.predicates.into_iter().filter_map(|(p, _)| p.to_opt_poly_trait_ref()); | |||
let predicates = &self.get_type_parameter_bounds(span, ty_param_def_id).predicates; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, it's cool that this works! (I think it's a case of the generalization of let x = &f();
)
src/librustc/ty/mod.rs
Outdated
let predicates = tcx.predicates_of(self.did).predicates; | ||
if predicates.into_iter().any(|(p, _)| p == sized_predicate) { | ||
let predicates = &tcx.predicates_of(self.did).predicates; | ||
if predicates.into_iter().any(|(p, _)| *p == sized_predicate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You changed into_iter
to iter
elsewhere, can you do it here too?
@@ -127,7 +127,7 @@ define_queries! { <'tcx> | |||
/// predicate gets in the way of some checks, which are intended | |||
/// to operate over only the actual where-clauses written by the | |||
/// user.) | |||
[] fn predicates_of: PredicatesOfItem(DefId) -> ty::GenericPredicates<'tcx>, | |||
[] fn predicates_of: PredicatesOfItem(DefId) -> Lrc<ty::GenericPredicates<'tcx>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also change predicates_defined_on
and type_param_predicates
.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@eddyb: I've updated, addressing the comments, and converting the remaining |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
parent: None, | ||
predicates: vec![], | ||
}, | ||
}), | ||
|parent| { | ||
let icx = ItemCtxt::new(tcx, parent); | ||
icx.get_type_parameter_bounds(DUMMY_SP, def_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you clone the GenericPredicates
here you don't need an extra variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, but maybe we could make it read a bit nicer? I've found that this use-case is very well handled by the make_mut
family of functions; it might even be worth adding an extend
or something that first invokes make_mut
, so that we can just do predicates.extend(...)
and make it just work. (Key thing would be to avoid doing anything if there are no items to extend with)
Thoughts?
src/librustc_typeck/collect.rs
Outdated
icx.type_parameter_bounds_in_generics(ast_generics, param_id, ty, OnlySelfBounds(true))); | ||
|
||
let mut preds = result.predicates.clone(); | ||
preds.extend(extra_preds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we maybe check if extra_preds.is_empty
and avoid the clone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't quite tell what the type of result
is here though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct solution is make_mut
, IMO, as discussed on IRC.
src/librustc_typeck/collect.rs
Outdated
let explicit = tcx.explicit_predicates_of(def_id); | ||
let span = tcx.def_span(def_id); | ||
let predicates = explicit.predicates.into_iter().chain( | ||
let predicates = explicit.predicates.iter().cloned().chain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of time, inferred_outlives_of
will be empty -- maybe we should check for that?
If we added Lrc::make_mut
, we could do:
let mut predicates = tcx.explicit_predicates_of(def_id);
let inferred_outlives = tcx.inferred_outlives_of(def_id);
if !inferred_outlives.is_empty() {
let predicates = Lrc::make_mut(&mut predicates);
predicates.predicates.extend(inferred_outlives.iter().map(|&p| (p, span)));
}
or something like that
src/librustc_typeck/collect.rs
Outdated
let span = tcx.def_span(def_id); | ||
predicates.push((ty::TraitRef::identity(tcx, def_id).to_predicate(), span)); | ||
} | ||
preds.push((ty::TraitRef::identity(tcx, def_id).to_predicate(), span)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here we could do this more elegant with make_mut
I think
d98c0c9
to
7b25382
Compare
I have updated the code to use r? @eddyb |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors try |
⌛ Trying commit 7b25382ed1210a942d06b01d6861853d874c2fda with merge 0efff4d7654f6a01ae707446fe13f85adaf93b0e... |
☀️ Test successful - status-travis |
7b25382
to
6a52d8f
Compare
Updated to address the most recent comment. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #55649) made this pull request unmergeable. Please resolve the merge conflicts. |
6a52d8f
to
5ddf50d
Compare
I rebased and reinstated the @bors r=eddyb |
📌 Commit 5ddf50d18b30b3a3fe2a308be3d74c596ed5adf6 has been approved by |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
So that the frequent clones in `try_get` are cheaper. Fixes rust-lang#54274.
5ddf50d
to
98dab33
Compare
Oh, that @bors r=eddyb |
📌 Commit 98dab33 has been approved by |
Wrap some query results in `Lrc`. So that the frequent clones in `try_get` are cheaper.
Rollup of 17 pull requests Successful merges: - #55182 (Redox: Update to new changes) - #55211 (Add BufWriter::buffer method) - #55507 (Add link to std::mem::size_of to size_of intrinsic documentation) - #55530 (Speed up String::from_utf16) - #55556 (Use `Mmap` to open the rmeta file.) - #55622 (NetBSD: link libstd with librt in addition to libpthread) - #55750 (Make `NodeId` and `HirLocalId` `newtype_index`) - #55778 (Wrap some query results in `Lrc`.) - #55781 (More precise spans for temps and their drops) - #55785 (Add mem::forget_unsized() for forgetting unsized values) - #55852 (Rewrite `...` as `..=` as a `MachineApplicable` 2018 idiom lint) - #55865 (Unix RwLock: avoid racy access to write_locked) - #55901 (fix various typos in doc comments) - #55926 (Change sidebar selector to fix compatibility with docs.rs) - #55930 (A handful of hir tweaks) - #55932 (core/char: Speed up `to_digit()` for `radix <= 10`) - #55956 (add tests for some fixed ICEs) Failed merges: r? @ghost
So that the frequent clones in
try_get
are cheaper.