Split the type context into a global and a local (inference-only) one. #33425

eddyb · 2016-05-05T04:51:56Z

After this change, each InferCtxt creates its own local type interner for types with inference by-products.
Most of the code which handles both a global and a local interner uses 'gcx and 'tcx for them.
A reference to the type context in that situation (e.g. infcx.tcx) is TyCtxt<'a, 'gcx, 'tcx>.
The global type context which used to be &'a TyCtxt<'tcx> is now TyCtxt<'a, 'tcx, 'tcx>.

In order to minimize the number of extra lifetime parameters, many functions became methods.
Where possible (some inherent impls), lifetime parameters were added on the impl, not each method.

As inference by-products no longer escape their inference contexts, memory usage is lower.
Example of -Z time-passes excerpt for librustc, stage1 (~100MB gains):
Before "rustc: Split local type contexts interners from the global one.":

time: 0.395; rss: 335MB item-types checking
time: 15.392; rss: 472MB        item-bodies checking
time: 0.000; rss: 472MB drop-impl checking
time: 1.140; rss: 478MB const checking
time: 0.139; rss: 478MB privacy checking
time: 0.024; rss: 478MB stability index
time: 0.072; rss: 478MB intrinsic checking
time: 0.038; rss: 478MB effect checking
time: 0.255; rss: 478MB match checking
time: 0.128; rss: 484MB liveness checking
time: 1.372; rss: 484MB rvalue checking
time: 1.404; rss: 597MB MIR dump
time: 0.809; rss: 599MB MIR passes

After:

time: 0.467; rss: 337MB item-types checking
time: 17.443; rss: 395MB        item-bodies checking
time: 0.000; rss: 395MB drop-impl checking
time: 1.423; rss: 401MB const checking
time: 0.141; rss: 401MB privacy checking
time: 0.024; rss: 401MB stability index
time: 0.116; rss: 401MB intrinsic checking
time: 0.038; rss: 401MB effect checking
time: 0.382; rss: 401MB match checking
time: 0.132; rss: 407MB liveness checking
time: 1.678; rss: 407MB rvalue checking
time: 1.614; rss: 503MB MIR dump
time: 0.957; rss: 512MB MIR passes

NOTE: Functions changed to methods weren't re-indented to keep this PR easier to review.
Once approved, the changes will be mechanically performed.
However, indentation changes of function arguments are there - and I believe there's a way to hide whitespace-only changes in diffs on GitHub.

rust-highfive · 2016-05-05T04:52:09Z

r? @nikomatsakis

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

durka · 2016-05-05T05:24:04Z

Nice memory savings, but 3.5x time regression in item-bodies checking?

eddyb · 2016-05-05T05:26:44Z

@durka Just noticed that actually, will need to investigate. We might have to make lifting cheaper.
EDIT: I also could have accidentally broken one of the trait selection caches, for example.

eddyb · 2016-05-05T20:12:00Z

About 25% of the time appears to be spend in malloc and free.
I've tried reusing arenas across function-local type contexts during type-checking (internally each TypedArena can reuse only the largest chunk it holds) but it doesn't appear to help.

arielb1 · 2016-05-05T20:13:06Z

What are the callers?

eddyb · 2016-05-05T20:18:41Z

@arielb1 I believe I'd need perf record -g for that and DWARF debug info and last time I tried something like that it was writing GBs of traces. I'm going to have to do it anyway - I'll try to look into it in the next few days.

eddyb · 2016-05-06T05:51:48Z

I've found a simple way of turning the split off while keeping all of the infrastructural changes, if we want to merge this without any significant impact on performance.

But it turns out that making tcx.lift(x) return None if x.needs_infer() && tcx.is_global() is enough to trigger the performance problem, without actually using the local interner at all.

So either the cost of checking needs_infer is really that high, or I broke one of the caches which depend on it.

eddyb · 2016-05-06T06:20:10Z

@nikomatsakis I'm pretty sure the problem here is the global selection/evaluation caches which used to allow TyInfer(FreshTy(_)) and friends but those have HAS_TY_INFER just like any other inference variables so they get stuck in the local type contexts.

Should I just add a separate flag for freshened types, and keep them in the global context, just for the caches?

nikomatsakis · 2016-05-06T08:59:40Z

@eddyb

Should I just add a separate flag for freshened types, and keep them in the global context, just for the caches?

Hmm. Seems worth a try! I'm a bit wary of removing TY_INFER from freshened types -- that is, I'm frightened that there will be existing logic that was assuming that they count. We could I guess add a new flag for "local context only" that is set for all ty-infer except freshened types.

eddyb · 2016-05-06T09:02:29Z

@nikomatsakis I'd keep the current behavior of needs_infer and friends and only ignore the TY_FRESH flag when checking whether something must be interned in the local type context.

eddyb · 2016-05-06T09:42:11Z

@durka @arielb1 Now that I solved the cache problem, the times look good again, although they all seem slightly increased.

bors · 2016-05-06T15:39:15Z

☔ The latest upstream changes (presumably #33138) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2016-05-07T03:30:56Z

☔ The latest upstream changes (presumably #33267) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2016-05-08T14:02:28Z

☔ The latest upstream changes (presumably #33091) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2016-05-10T01:27:05Z

☔ The latest upstream changes (presumably #33523) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2016-05-10T20:52:46Z

@eddyb r=me

you are my hero 👑

bors · 2016-05-11T05:48:41Z

📌 Commit 42eb703 has been approved by nikomatsakis

bors · 2016-05-11T06:40:10Z

⌛ Testing commit 42eb703 with merge c7ab884...

Split the type context into a global and a local (inference-only) one. After this change, each `InferCtxt` creates its own local type interner for types with inference by-products. Most of the code which handles both a global and a local interner uses `'gcx` and `'tcx` for them. A reference to the type context in that situation (e.g. `infcx.tcx`) is `TyCtxt<'a, 'gcx, 'tcx>`. The global type context which used to be `&'a TyCtxt<'tcx>` is now `TyCtxt<'a, 'tcx, 'tcx>`. In order to minimize the number of extra lifetime parameters, many functions became methods. Where possible (some inherent impls), lifetime parameters were added on the impl, not each method. As inference by-products no longer escape their inference contexts, memory usage is lower. Example of `-Z time-passes` excerpt for `librustc`, stage1 (~100MB gains): Before "rustc: Split local type contexts interners from the global one.": ``` time: 0.395; rss: 335MB item-types checking time: 15.392; rss: 472MB item-bodies checking time: 0.000; rss: 472MB drop-impl checking time: 1.140; rss: 478MB const checking time: 0.139; rss: 478MB privacy checking time: 0.024; rss: 478MB stability index time: 0.072; rss: 478MB intrinsic checking time: 0.038; rss: 478MB effect checking time: 0.255; rss: 478MB match checking time: 0.128; rss: 484MB liveness checking time: 1.372; rss: 484MB rvalue checking time: 1.404; rss: 597MB MIR dump time: 0.809; rss: 599MB MIR passes ``` After: ``` time: 0.467; rss: 337MB item-types checking time: 17.443; rss: 395MB item-bodies checking time: 0.000; rss: 395MB drop-impl checking time: 1.423; rss: 401MB const checking time: 0.141; rss: 401MB privacy checking time: 0.024; rss: 401MB stability index time: 0.116; rss: 401MB intrinsic checking time: 0.038; rss: 401MB effect checking time: 0.382; rss: 401MB match checking time: 0.132; rss: 407MB liveness checking time: 1.678; rss: 407MB rvalue checking time: 1.614; rss: 503MB MIR dump time: 0.957; rss: 512MB MIR passes ``` **NOTE**: Functions changed to methods weren't re-indented to keep this PR easier to review. Once approved, the changes will be mechanically performed. However, indentation changes of function arguments are there - and I believe there's a way to hide whitespace-only changes in diffs on GitHub.

bors · 2016-05-11T09:59:50Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

It turns out that `ast_ty_to_ty` is supposed to be updating the `def` after it finishes, but at some point in the past it stopped doing so. This was never noticed because of the `ast_ty_to_ty_cache`, but that cache was recently removed. This PR fixes the code to update the def properly, but apparently that is not quite enough to make the operation idempotent, so for now we reintroduce the cache too. Fixes rust-lang#33425.

@eddyb

…-type-path, r=eddyb re-introduce a cache for ast-ty-to-ty It turns out that `ast_ty_to_ty` is supposed to be updating the `def` after it finishes, but at some point in the past it stopped doing so. This was never noticed because of the `ast_ty_to_ty_cache`, but that cache was recently removed. This PR fixes the code to update the def properly, but apparently that is not quite enough to make the operation idempotent, so for now we reintroduce the cache too. Fixes rust-lang#33425. r? @eddyb

rust-highfive assigned nikomatsakis May 5, 2016

eddyb force-pushed the rift branch from 5de9234 to acbfa9a Compare May 5, 2016 07:19

eddyb force-pushed the rift branch from cafbf22 to e9e1804 Compare May 6, 2016 05:46

eddyb force-pushed the rift branch from e9e1804 to abc18b3 Compare May 6, 2016 09:40

eddyb force-pushed the rift branch from abc18b3 to d1f7d57 Compare May 7, 2016 01:43

eddyb force-pushed the rift branch 2 times, most recently from e2a30af to 3316c7a Compare May 7, 2016 16:15

eddyb force-pushed the rift branch from 3316c7a to 3cd3fee Compare May 9, 2016 07:19

eddyb force-pushed the rift branch from 3cd3fee to f62aece Compare May 11, 2016 01:14

eddyb added 5 commits May 11, 2016 04:14

rustc: Always refer to TyCtxt as tcx.

b5122d5

infer: Use methods for creating an InferCtxt.

0907c19

infer: Turn normalize_associated_type into a method on TyCtxt.

6e29099

mem_categorization: freely_aliasable doesn't need to take TyCtxt.

8600a67

typeck: merge CollectCtxt and collect::CollectCtxt.

e387e6c

bors merged commit 42eb703 into rust-lang:master May 11, 2016

eddyb deleted the rift branch May 11, 2016 15:11

mcarton mentioned this pull request May 12, 2016

Rustup to *1.10.0-nightly (22ac88f1a 2016-05-11)* rust-lang/rust-clippy#922

Merged

nikomatsakis mentioned this pull request May 12, 2016

re-introduce a cache for ast-ty-to-ty #33596

Merged

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label May 13, 2016

eddyb mentioned this pull request Jun 4, 2016

many major compile-time perf regressions in jld-day15-parser #34042

Closed

TimNN mentioned this pull request Oct 20, 2016

ICE with cross-crate projection #37291

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the type context into a global and a local (inference-only) one. #33425

Split the type context into a global and a local (inference-only) one. #33425

eddyb commented May 5, 2016 •

edited

Loading

rust-highfive commented May 5, 2016

durka commented May 5, 2016

eddyb commented May 5, 2016 •

edited

Loading

eddyb commented May 5, 2016

arielb1 commented May 5, 2016

eddyb commented May 5, 2016 •

edited

Loading

eddyb commented May 6, 2016

eddyb commented May 6, 2016

nikomatsakis commented May 6, 2016

eddyb commented May 6, 2016

eddyb commented May 6, 2016

bors commented May 6, 2016

bors commented May 7, 2016

bors commented May 8, 2016

bors commented May 10, 2016

nikomatsakis commented May 10, 2016

bors commented May 11, 2016

bors commented May 11, 2016

bors commented May 11, 2016

Split the type context into a global and a local (inference-only) one. #33425

Split the type context into a global and a local (inference-only) one. #33425

Conversation

eddyb commented May 5, 2016 • edited Loading

rust-highfive commented May 5, 2016

durka commented May 5, 2016

eddyb commented May 5, 2016 • edited Loading

eddyb commented May 5, 2016

arielb1 commented May 5, 2016

eddyb commented May 5, 2016 • edited Loading

eddyb commented May 6, 2016

eddyb commented May 6, 2016

nikomatsakis commented May 6, 2016

eddyb commented May 6, 2016

eddyb commented May 6, 2016

bors commented May 6, 2016

bors commented May 7, 2016

bors commented May 8, 2016

bors commented May 10, 2016

nikomatsakis commented May 10, 2016

bors commented May 11, 2016

bors commented May 11, 2016

bors commented May 11, 2016

eddyb commented May 5, 2016 •

edited

Loading

eddyb commented May 5, 2016 •

edited

Loading

eddyb commented May 5, 2016 •

edited

Loading