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

Speed up `opt_normalize_projection_type` #50818

Merged
merged 2 commits into from May 18, 2018

Conversation

Projects
None yet
7 participants
@nnethercote
Contributor

nnethercote commented May 17, 2018

opt_normalize_projection_type is hot in the serde and futures benchmarks in rustc-perf. These two patches speed up the execution of most runs for them by 2--4%.

nnethercote added some commits May 16, 2018

Avoid repeated HashMap lookups in `opt_normalize_projection_type`.
There is a hot path through `opt_normalize_projection_type`:
- `try_start` does a cache lookup (#1).
- The result is a `NormalizedTy`.
- There are no unresolved type vars, so we call `complete`.
- `complete` does *another* cache lookup (#2), then calls
  `SnapshotMap::insert`.
- `insert` does *another* cache lookup (#3), inserting the same value
  that's already in the cache.

This patch optimizes this hot path by introducing `complete_normalized`,
for use when the value is known in advance to be a `NormalizedTy`. It
always avoids lookup #2. Furthermore, if the `NormalizedTy`'s
obligations are empty (the common case), we know that lookup #3 would be
a no-op, so we avoid it, while inserting a Noop into the `SnapshotMap`'s
undo log.
Avoid allocations in `opt_normalize_projection_type`.
This patch changes `opt_normalize_project_type` so it appends
obligations to a given obligations vector, instead of returning a new
obligations vector.

This change avoids lots of allocations. In the most extreme case, for a
clean "Check" build of serde it reduces the total number of allocations
by 20%.
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented May 17, 2018

r? @estebank

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

@@ -202,17 +202,19 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
obligation.cause.span,
infer::LateBoundRegionConversionTime::HigherRankedType,
data);
let normalized = super::normalize_projection_type(
let mut obligations = vec![];

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum May 17, 2018

Member

If this specifically is hot, it'd be interesting to see if we could make this an Option argument since obligations appear to be unused in this case.

This comment has been minimized.

@nnethercote

nnethercote May 17, 2018

Contributor

This particular callsite is not hot. The hot callsite is the one within <TypeFolder for AssociatedTypeNormalizer>::fold_ty in librustc/traits/project.rs, which passes an existing vector.

@eddyb

This comment has been minimized.

Member

eddyb commented May 17, 2018

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 17, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented May 17, 2018

📌 Commit 47bc774 has been approved by nikomatsakis

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented May 17, 2018

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 17, 2018

Rollup merge of rust-lang#50818 - nnethercote:faster-normalize, r=nik…
…omatsakis

Speed up `opt_normalize_projection_type`

`opt_normalize_projection_type` is hot in the serde and futures benchmarks in rustc-perf. These two patches speed up the execution of most runs for them by 2--4%.

bors added a commit that referenced this pull request May 18, 2018

Auto merge of #50847 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
Rollup of 10 pull requests

Successful merges:

 - #50387 (Remove leftover tab in libtest outputs)
 - #50553 (Add Option::xor method)
 - #50610 (Improve format string errors)
 - #50649 (Tweak `nearest_common_ancestor()`.)
 - #50790 (Fix grammar documentation wrt Unicode identifiers)
 - #50791 (Fix null exclusions in grammar docs)
 - #50806 (Add `bless` x.py subcommand for easy ui test replacement)
 - #50818 (Speed up `opt_normalize_projection_type`)
 - #50837 (Revert #49767)
 - #50839 (Make sure people know the book is free oline)

Failed merges:

@bors bors merged commit 47bc774 into rust-lang:master May 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nnethercote nnethercote deleted the nnethercote:faster-normalize branch May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment