Skip to content
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

Improve memoization and refactor NLL type check #51460

Merged
merged 14 commits into from
Jun 18, 2018

Conversation

nikomatsakis
Copy link
Contributor

I have a big branch that is refactoring NLL type check with the goal of introducing canonicalization-based memoization for all of the operations it does. This PR contains an initial prefix of that branch which, I believe, stands alone. It does introduce a few smaller optimizations of its own:

  • Skip operations that are trivially a no-op
  • Cache the results of the dropck-outlives computations done by liveness
  • Skip resetting unifications if nothing changed

r? @pnkfelix

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 9, 2018
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jun 9, 2018

@bors try -- might be nice to do a perf run while we wait -- but I wouldn't block on it if @pnkfelix gets around to doing a review sooner

@bors
Copy link
Contributor

bors commented Jun 9, 2018

⌛ Trying commit 2e25bed with merge 757cd05...

bors added a commit that referenced this pull request Jun 9, 2018
…r=<try>

Improve memoization and refactor NLL type check

I have a big branch that is refactoring NLL type check with the goal of introducing canonicalization-based memoization for all of the operations it does. This PR contains an initial prefix of that branch which, I believe, stands alone. It does introduce a few smaller optimizations of its own:

- Skip operations that are trivially a no-op
- Cache the results of the dropck-outlives computations done by liveness
- Skip resetting unifications if nothing changed

r? @pnkfelix
@Mark-Simulacrum
Copy link
Member

Perf queued.

@bors
Copy link
Contributor

bors commented Jun 10, 2018

💥 Test timed out

@kennytm
Copy link
Member

kennytm commented Jun 10, 2018

Perf result is great for those that can be compiled. The timings for futures and serde are missing though.

http://perf.rust-lang.org/compare.html?start=61d88318aa66669fba061e9af529365172d63cd0&end=757cd050fc1ef84d7235d6f4d9228189eed878cc&stat=instructions%3Au

@Mark-Simulacrum
Copy link
Member

Compilation failures for serde/futures:

Serde

thread 'main' panicked at 'region_obligations not empty: [
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#16r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#17r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#18r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#19r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#20r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#21r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#22r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#23r, sup_type=M)
    ),
    (
        NodeId(
            18904
        ),
        RegionObligation(sub_region='_#24r, sup_type=M)
    )
]', librustc/infer/mod.rs:1057:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.28.0-nightly (757cd050f 2018-06-09) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z borrowck=mir -C debuginfo=2 --crate-type lib

note: some of the compiler flags provided by cargo are hidden

thread 'main' panicked at 'assertion failed: cmd.status().expect("failed to spawn").success()', collector/src/bin/rustc-fake.rs:33:17

futures

thread 'main' panicked at 'region_obligations not empty: [
    (
        NodeId(
            3805
        ),
        RegionObligation(sub_region=ReStatic, sup_type=U)
    ),
    (
        NodeId(
            3805
        ),
        RegionObligation(sub_region=ReStatic, sup_type=U)
    )
]', librustc/infer/mod.rs:1057:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.28.0-nightly (757cd050f 2018-06-09) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z borrowck=mir -C debuginfo=2 --crate-type lib

note: some of the compiler flags provided by cargo are hidden

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2018
@nikomatsakis
Copy link
Contributor Author

Interesting. Thanks!

@pnkfelix
Copy link
Member

(FYI I plan to start my review on Tuesday June 12th)

@nikomatsakis
Copy link
Contributor Author

Minimized failure:

struct Map<A, F> where A: Future {
    a: A,
    f: F,
}

trait Future {
    type Item;
    type Error;

    fn tailcall(&mut self) -> Option<Box<Future<Item=Self::Item, Error=Self::Error>>>;
}

impl<A, F, U> Future for Map<A, F>
where
    A: Future,
    F: FnOnce(A::Item) -> U + Send + 'static,
    U: Send + 'static,
{
    type Item = U;
    type Error = A::Error;

    fn tailcall(&mut self)
                -> Option<Box<Future<Item=Self::Item, Error=Self::Error>>> {
        None
    }
}

fn main() {}

I found the bug, it is a pre-existing weirdness in the NLL checker that I have to think about the best way to fix. =)

Copy link
Member

@pnkfelix pnkfelix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just from skimming 956e2f8 I'll just say it would be nice if the rustfmt runs were isolated to their own commits...)

((I am now experimenting with "ignore whitespace changes" to try to deal with this...))

(((oh, apparently there are commits that are factored out rustfmt runs. maybe 956e2f8 is just the result of IDE auto-indent...)))

@pnkfelix
Copy link
Member

The PR seems fine from what I saw. (Obviously I must have missed something since there is the aforementioned bug that niko is working on.)

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 18, 2018

📌 Commit 2e25bed has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 18, 2018
@bors
Copy link
Contributor

bors commented Jun 18, 2018

⌛ Testing commit 2e25bed with merge b36917b...

bors added a commit that referenced this pull request Jun 18, 2018
…r=pnkfelix

Improve memoization and refactor NLL type check

I have a big branch that is refactoring NLL type check with the goal of introducing canonicalization-based memoization for all of the operations it does. This PR contains an initial prefix of that branch which, I believe, stands alone. It does introduce a few smaller optimizations of its own:

- Skip operations that are trivially a no-op
- Cache the results of the dropck-outlives computations done by liveness
- Skip resetting unifications if nothing changed

r? @pnkfelix
@bors
Copy link
Contributor

bors commented Jun 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing b36917b to master...

@nnethercote
Copy link
Contributor

This gave some big perf wins for NLL perf, but also lots of smaller improvements for non-NLL builds:
http://perf.rust-lang.org/compare.html?start=862703e05e275d77b0b594bb5d1a26a6340933f2&end=b36917b33104dc0af4d26d53899b7cd064a40b22&stat=instructions:u

Nice work.

cc @rust-lang/wg-compiler-performance

bors added a commit that referenced this pull request Jun 25, 2018
[WIP] Convert NLL ops to caches

This is a extension of <#51460>. It uses a lot more caching than we used to do. This caching is not yet as efficient as it could be, but I'm curious to see the current perf results.
bors added a commit that referenced this pull request Jun 28, 2018
convert NLL ops to caches

This is a extension of <#51460>. It uses a lot more caching than we used to do. This caching is not yet as efficient as it could be, but I'm curious to see the current perf results.

This is the high-level idea: in the MIR type checker, use [canonicalized queries](https://rust-lang-nursery.github.io/rustc-guide/traits/canonical-queries.html) for all the major operations. This is helpful because the MIR type check is operating in a context where all types are fully known (mostly, anyway) but regions are completely renumbered. This means we often wind up with duplicate queries like `Foo<'1, '2> :Bar` and `Foo<'3, '4>: Bar`. Canonicalized queries let us re-use the results. By the final commit in this PR, we can essentially just "read off" the resulting region relations and add them to the NLL type check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants