Skip to content

/rust

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

introduce canonical queries, use for normalization and dropck-outlives #48411

Merged
merged 27 commits into from Mar 13, 2018

Conversation

Reviewers

@eddyb eddyb

@SimonSapin SimonSapin

@RalfJung RalfJung

@bjorn3 bjorn3

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 21, 2018 •
edited

This branch adds in the concept of a canonicalized trait query and uses it for three specific operations:

  • infcx.at(cause, param_env).normalize(type_foldable)
    • normalizes all associated types in type_foldable
  • tcx.normalize_erasing_regions(param_env, type_foldable)
    • like normalize, but erases regions first and in the result; this leads to better caching
  • infcx.at(cause, param_env).dropck_outlives(ty)
    • produces the set of types that must be live when a value of type ty is dropped
    • used from dropck but also NLL outlives

This is a kind of "first step" towards a more Chalk-ified approach. It leads to a big speedup for NLL, which is basically dominated by the dropck-outlives computation. Here are some timing measurements for the syn crate (pre-branch measurements coming soon):

Commit NLL disabled NLL enabled
Before my branch 5.43s 8.99s
After my branch 5.36s 7.25s

(Note that NLL enabled still does all the work that NLL disabled does, so this is not really a way to compare the performance of NLL versus the AST-based borrow checker directly.) Since this affects all codepaths, I'd like to do a full perf run before we land anything.

Also, this is not the "final point" for canonicalization etc. I think canonicalization can be made substantially faster, for one thing. But it seems like a reasonable starting point for a branch that's gotten a bit larger than I would have liked.

Commit convention: First of all, this entire branch ought to be a "pure refactoring", I believe, not changing anything about external behavior. Second, I've tagged the most important commits with [VIC] (very important commit), so you can scan for those. =)

r? @eddyb

@rust-highfive rust-highfive assigned eddyb Feb 21, 2018

@rust-highfive rust-highfive added the S-waiting-on-review label Feb 21, 2018

@eddyb

eddyb reviewed Feb 21, 2018
View changes

src/librustc_traits/normalize_erasing_regions.rs Outdated
let cause = ObligationCause::dummy();
match infcx.at(&cause, param_env).normalize(&value) {
Ok(Normalized { value: normalized_value, obligations: _ }) => {
// ^^^^^^^^^^^

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Feb 21, 2018

Member

How certain is this? Can you assert that this holds, or would that be too expensive?

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Feb 21, 2018

Author Contributor

Hmm. It's probably worth asserting, or maybe refactoring the types to prove it.
@eddyb

eddyb reviewed Feb 21, 2018
View changes

src/librustc_trans/mir/mod.rs Outdated
@@ -104,7 +103,7 @@ pub struct FunctionCx<'a, 'tcx:'a> {

impl<'a, 'tcx> FunctionCx<'a, 'tcx> {
pub fn monomorphize<T>(&self, value: &T) -> T
where T: TransNormalize<'tcx>
where T: TypeFoldable<'tcx>
{
self.cx.tcx.trans_apply_param_substs(self.param_substs, value)

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Feb 21, 2018

Member

Can we get rid of trans_apply_param_substs now?
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Feb 21, 2018

r=me with nits and build failure addressed

@nikomatsakis nikomatsakis referenced this pull request Feb 22, 2018

Open

🛠 improve canonicalization by using a hashing scheme #48417

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 22, 2018

@bors try

I'd like to do a perf run.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 22, 2018

⌛️ Trying commit 6d787d4 with merge fc2b29d...

bors added a commit that referenced this pull request Feb 22, 2018

@bors
Auto merge of #48411 - nikomatsakis:chalkify-canonical-query-mir, r=<… 
…try>

introduce canonical queries, use for normalization and dropck-outlives

This branch adds in the concept of a **canonicalized trait query** and uses it for three specific operations:

- `infcx.at(cause, param_env).normalize(type_foldable)`
    - normalizes all associated types in `type_foldable`
- `tcx.normalize_erasing_regions(param_env, type_foldable)`
    - like normalize, but erases regions first and in the result; this leads to better caching
- `infcx.at(cause, param_env).dropck_outlives(ty)`
    - produces the set of types that must be live when a value of type `ty` is dropped
    - used from dropck but also NLL outlives

This is a kind of "first step" towards a more Chalk-ified approach. It leads to a **big** speedup for NLL, which is basically dominated by the dropck-outlives computation. Here are some timing measurements for the `syn` crate (pre-branch measurements coming soon):

| Commit | NLL disabled | NLL enabled |
| ------- | --- | --- |
| Before my branch | 5.43s | 8.99s |
| After my branch | 5.36s | 7.25s |

(Note that NLL enabled still does *all the work* that NLL disabled does, so this is not really a way to compare the performance of NLL versus the AST-based borrow checker directly.) Since this affects all codepaths, I'd like to do a full perf run before we land anything.

Also, this is not the "final point" for canonicalization etc. I think canonicalization can be made substantially faster, for one thing. But it seems like a reasonable starting point for a branch that's gotten a bit larger than I would have liked.

**Commit convention:** First of all, this entire branch ought to be a "pure refactoring", I believe, not changing anything about external behavior. Second, I've tagged the most important commits with `[VIC]` (very important commit), so you can scan for those. =)

r? @eddyb
Loading status checks…
fc2b29d

@nikomatsakis nikomatsakis referenced this pull request Feb 22, 2018

Closed

NLL performance tracking issue #47879

2 of 6 tasks complete
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 22, 2018

☀️ Test successful - status-travis
State: approved= try=True
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 22, 2018

cc @rust-lang/release -- I'd like to request a perf run. try build completed, not sure what other steps I need to do.

@pietroalbini pietroalbini added S-waiting-on-author and removed S-waiting-on-review labels Feb 22, 2018

@Mark-Simulacrum

This comment has been minimized.

Sign in to view
Copy link
Member

Mark-Simulacrum commented Feb 22, 2018

I'll run it soon, for now just always ping me
@Mark-Simulacrum

This comment has been minimized.

Sign in to view
Copy link
Member

Mark-Simulacrum commented Feb 22, 2018

Perf started
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 22, 2018 •
edited

Here are the results:

perf comparison

Also archived here, since I've noticed that perf links tend to go stale:

http://smallcultfollowing.com/pr-48411/rustc%20performance%20data.htm (instructions)
http://smallcultfollowing.com/pr-48411/times.html (times)
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 22, 2018

Major changes (>10%)

Got worse:

  • futures/futures-opt: +10-15%
  • regex: 34.4% (patched incremental: compile one)
  • regex-opt: 56% (patched incremental: compile one-opt)

Got better

  • crates.io-opt: -30.0% on println-opt
  • regression-31157, regression-31157-opt: -10% or so
  • style-servo-opt: -21.0% (clean-incremental-opt)

Medium changes (3-10%)

  • coercions/coercions-opt: +4%
  • piston-image: +6% (clean-incremental, clean-incremental-opt)
  • unify-linearly: +9% (clean)

Got better:

  • tokio-webpush-simple: -5%
  • tokio-webpush-simple-opt: -2% through -5%
  • unify-linearly: -4% (dummy fn)

Small changes (<3%)

  • syn: -2%
  • crates.io: -2.5% mostly (one change +1%)
  • deep-vector: +1%
  • htm5ever: +2%
  • html5ever-opt: +1%
  • hyper: +1%, -2%
  • parser, parser-opt: +2%
  • piston-image: -1%, +6%
  • tuple-stress: +2%
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 22, 2018

I'll try to take a look at regex and regex-opt.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 23, 2018

@bors try
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 23, 2018

🔒 Merge conflict

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:chalkify-canonical-query-mir branch 2 times, most recently from 0e84c2d to 41d762f Feb 23, 2018

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 24, 2018

@bors try

bors added a commit that referenced this pull request Feb 24, 2018

@bors
Auto merge of #48411 - nikomatsakis:chalkify-canonical-query-mir, r=<… 
…try>

introduce canonical queries, use for normalization and dropck-outlives

This branch adds in the concept of a **canonicalized trait query** and uses it for three specific operations:

- `infcx.at(cause, param_env).normalize(type_foldable)`
    - normalizes all associated types in `type_foldable`
- `tcx.normalize_erasing_regions(param_env, type_foldable)`
    - like normalize, but erases regions first and in the result; this leads to better caching
- `infcx.at(cause, param_env).dropck_outlives(ty)`
    - produces the set of types that must be live when a value of type `ty` is dropped
    - used from dropck but also NLL outlives

This is a kind of "first step" towards a more Chalk-ified approach. It leads to a **big** speedup for NLL, which is basically dominated by the dropck-outlives computation. Here are some timing measurements for the `syn` crate (pre-branch measurements coming soon):

| Commit | NLL disabled | NLL enabled |
| ------- | --- | --- |
| Before my branch | 5.43s | 8.99s |
| After my branch | 5.36s | 7.25s |

(Note that NLL enabled still does *all the work* that NLL disabled does, so this is not really a way to compare the performance of NLL versus the AST-based borrow checker directly.) Since this affects all codepaths, I'd like to do a full perf run before we land anything.

Also, this is not the "final point" for canonicalization etc. I think canonicalization can be made substantially faster, for one thing. But it seems like a reasonable starting point for a branch that's gotten a bit larger than I would have liked.

**Commit convention:** First of all, this entire branch ought to be a "pure refactoring", I believe, not changing anything about external behavior. Second, I've tagged the most important commits with `[VIC]` (very important commit), so you can scan for those. =)

r? @eddyb
Loading status checks…
251f865
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 24, 2018

⌛️ Trying commit 41d762f with merge 251f865...

@eternaleye eternaleye referenced this pull request Feb 24, 2018

Open

Parametrise modules #424

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 24, 2018

☀️ Test successful - status-travis
State: approved= try=True
@Mark-Simulacrum

This comment has been minimized.

Sign in to view
Copy link
Member

Mark-Simulacrum commented Feb 24, 2018

Perf started for 251f865.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 24, 2018

@Mark-Simulacrum when I try to compare,I get errors about not having results for 063deba ... am I doing something wrong ? =)

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:chalkify-canonical-query-mir branch from fbaf9cb to 17c4103 Mar 13, 2018

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 13, 2018

@bors r=eddyb
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 13, 2018

📌 Commit 17c4103 has been approved by eddyb

@bors bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 13, 2018

@alexcrichton alexcrichton referenced this pull request Mar 13, 2018

Merged

Update sccache to its master branch #48647

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 13, 2018

⌛️ Testing commit 17c4103 with merge 8c4ff22...

bors added a commit that referenced this pull request Mar 13, 2018

@bors
Auto merge of #48411 - nikomatsakis:chalkify-canonical-query-mir, r=e… 
…ddyb

introduce canonical queries, use for normalization and dropck-outlives

This branch adds in the concept of a **canonicalized trait query** and uses it for three specific operations:

- `infcx.at(cause, param_env).normalize(type_foldable)`
    - normalizes all associated types in `type_foldable`
- `tcx.normalize_erasing_regions(param_env, type_foldable)`
    - like normalize, but erases regions first and in the result; this leads to better caching
- `infcx.at(cause, param_env).dropck_outlives(ty)`
    - produces the set of types that must be live when a value of type `ty` is dropped
    - used from dropck but also NLL outlives

This is a kind of "first step" towards a more Chalk-ified approach. It leads to a **big** speedup for NLL, which is basically dominated by the dropck-outlives computation. Here are some timing measurements for the `syn` crate (pre-branch measurements coming soon):

| Commit | NLL disabled | NLL enabled |
| ------- | --- | --- |
| Before my branch | 5.43s | 8.99s |
| After my branch | 5.36s | 7.25s |

(Note that NLL enabled still does *all the work* that NLL disabled does, so this is not really a way to compare the performance of NLL versus the AST-based borrow checker directly.) Since this affects all codepaths, I'd like to do a full perf run before we land anything.

Also, this is not the "final point" for canonicalization etc. I think canonicalization can be made substantially faster, for one thing. But it seems like a reasonable starting point for a branch that's gotten a bit larger than I would have liked.

**Commit convention:** First of all, this entire branch ought to be a "pure refactoring", I believe, not changing anything about external behavior. Second, I've tagged the most important commits with `[VIC]` (very important commit), so you can scan for those. =)

r? @eddyb
Loading status checks…
8c4ff22

@nikomatsakis nikomatsakis referenced this pull request Mar 13, 2018

Merged

NLL should identify and respect the lifetime annotations that the user wrote #48482

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 8c4ff22 to master...

@bors bors merged commit 17c4103 into rust-lang:master Mar 13, 2018
2 checks passed

2 checks passed

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

This was referenced Mar 13, 2018

Closed
Merged
Merged
Merged
Merged
Merged

@aravind-pg aravind-pg referenced this pull request Mar 13, 2018

Merged

Create a canonical trait query for `evaluate_obligation` #48995

@nikomatsakis nikomatsakis referenced this pull request Mar 13, 2018

Closed

NLL should identify and respect the lifetime annotations that the user wrote #47184

22 of 27 tasks complete

bors added a commit that referenced this pull request Mar 20, 2018

@bors
Auto merge of #48995 - aravind-pg:canonical-query, r=<try> 
[WIP] Create a canonical trait query for `evaluate_obligation`

This builds on the canonical query machinery introduced in #48411 to introduce a new canonical trait query for `evaluate_obligation` in the trait selector. Also ports most callers of the original `evaluate_obligation` to the new system (except in coherence, which requires support for intercrate mode). Closes #48537.

r? @nikomatsakis
Loading status checks…
0fdc38f

bors added a commit that referenced this pull request Mar 20, 2018

@bors
Auto merge of #48995 - aravind-pg:canonical-query, r=<try> 
[WIP] Create a canonical trait query for `evaluate_obligation`

This builds on the canonical query machinery introduced in #48411 to introduce a new canonical trait query for `evaluate_obligation` in the trait selector. Also ports most callers of the original `evaluate_obligation` to the new system (except in coherence, which requires support for intercrate mode). Closes #48537.

r? @nikomatsakis
Loading status checks…
09aab36

@davidtwco davidtwco referenced this pull request Apr 3, 2018

Closed

NLL breaks functions with intermingled lifetimes in return type when returned value is a (non-borrowed) associated type #49354

@pietroalbini pietroalbini referenced this pull request Apr 17, 2018

Closed

[ICE] librustc_traits/normalize_erasing_regions.rs:46: could not fully normalize `...` #49685

bors added a commit that referenced this pull request Apr 27, 2018

@bors
Auto merge of #48995 - aravind-pg:canonical-query, r=nikomatsakis 
Create a canonical trait query for `evaluate_obligation`

This builds on the canonical query machinery introduced in #48411 to introduce a new canonical trait query for `evaluate_obligation` in the trait selector. Also ports most callers of the original `evaluate_obligation` to the new system (except in coherence, which requires support for intercrate mode). Closes #48537.

r? @nikomatsakis
Loading status checks…
8a09bc6
@RalfJung

This comment has been minimized.

Sign in to view
Copy link
Member

RalfJung commented Jul 29, 2018 •
edited

FWIW, this (specifically, short-circuit dropck_outlives for simple cases ) caused a soundness bug: #52786
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.