Avoid useless Vec clones in pending_obligations(). #51412
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
(whoops, didn't mean to submit that. sorry for the noise!) |
| pub fn pending_obligations(&self) -> Vec<O> | ||
| where O: Clone | ||
| pub fn map_pending_obligations<P, F>(&self, f: F) -> Vec<P> | ||
| where O: Clone, |
There was a problem hiding this comment.
That Clone bound isn't necessary anymore
The only instance of `ObligationForest` in use has an obligation type of `PendingPredicateObligation`, which contains a `PredicateObligation` and a `Vec<Ty>`. `FulfillmentContext::pending_obligations()` calls `ObligationForest::pending_obligations()`, which clones all the `PendingPredicateObligation`s. But the `Vec<Ty>` field of those cloned obligations is never touched. This patch changes `ObligationForest::pending_obligations()` to `map_pending_obligations` -- which gives callers control about which part of the obligation to clone -- and takes advantage of the change to avoid cloning the `Vec<Ty>`. The change speeds up runs of a few rustc-perf benchmarks, the best by 1%.
nnethercote
force-pushed
the
pending_obligations
branch
from
18af5c6 to
b0440d3
Compare
Good catch. I've updated to remove it. |
| @@ -257,7 +256,6 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
| let expected_kind = fulfillment_cx | |||
| .pending_obligations() | |||
There was a problem hiding this comment.
If my grepping is accurate, these are the only two call sites. Since both call iter() on the resultant vector, can we not change the method to return an iterator directly, instead of collecting?
Further, I feel like we might be able to avoid cloning the obligation entirely, since iter() would only give reference-based access to it anyway...
There was a problem hiding this comment.
I tried that originally, but hit this error: "impl Trait not allowed outside of function and inherent method return types".
Because pending_obligations is a method of the TraitEngine trait.
There was a problem hiding this comment.
Ah, unfortunate. If this is hot enough, we could write a struct that implements iterator... but I'm not sure it's worth it.
There was a problem hiding this comment.
I think this patch got most of the potential gain.
|
@bors r+ |
|
📌 Commit b0440d3 has been approved by |
bors
added
S-waiting-on-bors
…estebank Avoid useless Vec clones in pending_obligations(). The only instance of `ObligationForest` in use has an obligation type of `PendingPredicateObligation`, which contains a `PredicateObligation` and a `Vec<Ty>`. `FulfillmentContext::pending_obligations()` calls `ObligationForest::pending_obligations()`, which clones all the `PendingPredicateObligation`s. But the `Vec<Ty>` field of those cloned obligations is never touched. This patch changes `ObligationForest::pending_obligations()` to `map_pending_obligations` -- which gives callers control about which part of the obligation to clone -- and takes advantage of the change to avoid cloning the `Vec<Ty>`. The change speeds up runs of a few rustc-perf benchmarks, the best by 1%.
Rollup of 13 pull requests Successful merges: - #50143 (Add deprecation lint for duplicated `macro_export`s) - #51099 (Fix Issue 38777) - #51276 (Dedup auto traits in trait objects.) - #51298 (Stabilize unit tests with non-`()` return type) - #51360 (Suggest parentheses when a struct literal needs them) - #51391 (Use spans pointing at the inside of a rustdoc attribute) - #51394 (Use scope tree depths to speed up `nearest_common_ancestor`.) - #51396 (Make the size of Option<NonZero*> a documented guarantee.) - #51401 (Warn on `repr` without hints) - #51412 (Avoid useless Vec clones in pending_obligations().) - #51427 (compiletest: autoremove duplicate .nll.* files (#51204)) - #51436 (Do not require stage 2 compiler for rustdoc) - #51437 (rustbuild: generate full list of dependencies for metadata) Failed merges:
The only instance of
ObligationForestin use has an obligation type ofPendingPredicateObligation, which contains aPredicateObligationand aVec<Ty>.FulfillmentContext::pending_obligations()callsObligationForest::pending_obligations(), which clones all thePendingPredicateObligations. But theVec<Ty>field of those clonedobligations is never touched.
This patch changes
ObligationForest::pending_obligations()tomap_pending_obligations-- which gives callers control about which partof the obligation to clone -- and takes advantage of the change to avoid
cloning the
Vec<Ty>. The change speeds up runs of a few rustc-perfbenchmarks, the best by 1%.