Wrap some query results in Lrc.#55778
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Unnecessary clone, can just keep the whole Lrc in the predicates variable and change the loop below.
There was a problem hiding this comment.
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.
Ok, I worked out a different way to do it.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think the .map(|p| *p) here and elsewhere could be replaced by .cloned().
There was a problem hiding this comment.
Won't that cause additional refcounting that the .map(|p| *p) won't?
There was a problem hiding this comment.
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.
src/librustc_typeck/astconv.rs
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You changed into_iter to iter elsewhere, can you do it here too?
src/librustc/ty/query/mod.rs
Outdated
There was a problem hiding this comment.
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 |
src/librustc_typeck/collect.rs
Outdated
There was a problem hiding this comment.
If you clone the GenericPredicates here you don't need an extra variable.
nikomatsakis
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
should we maybe check if extra_preds.is_empty and avoid the clone?
There was a problem hiding this comment.
I can't quite tell what the type of result is here though
There was a problem hiding this comment.
The correct solution is make_mut, IMO, as discussed on IRC.
src/librustc_typeck/collect.rs
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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_getare cheaper.