remove interior mutability of type-flags #42015

nikomatsakis · 2017-05-15T19:03:08Z

We were previously using the flags on Ty<'tcx> instances to do some ad-hoc caching schemes around things like is_sized(), is_freeze(), and moves_by_default(). This PR replaces those schemes with a proper query; the query key is based on the pair of a (ParameterEnvironment<'tcx>, Ty<'tcx>) pair. This is also intended to be a preliminary template for what trait-selection and projection will eventually look like.

I did some performance measurements. In the past, I observed a noticeable speedup (6%) for building rustc, but since I've rebased, the numbers appear to be more of a wash:

Crate	Before	After	Percentage
syntax	167s	166s	0.6% faster
rustc	376s	382s	1.5% slower

Some advantages of this new scheme:

is_sized etc are proper queries
we get caching across generic fns, so long as trait environment is identical
dependency tracking is correct

rust-highfive · 2017-05-15T19:03:12Z

r? @eddyb

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

nikomatsakis · 2017-05-15T19:03:51Z

cc @arielb1

eddyb · 2017-05-15T19:18:17Z

src/librustc/ty/mod.rs

@@ -507,8 +507,6 @@ bitflags! {
        // Caches for type_is_sized, type_moves_by_default
        const SIZEDNESS_CACHED  = 1 << 16,
        const IS_SIZED          = 1 << 17,
-        const MOVENESS_CACHED   = 1 << 18,
-        const MOVES_BY_DEFAULT  = 1 << 19,


Only two flags removed?

Mark-Simulacrum

Just some minor nits, no need to bother if you don't want to.

Mark-Simulacrum · 2017-05-15T19:53:14Z

src/librustc/infer/mod.rs

-        let param_env = param_env.take().unwrap_or_else(|| {
-            global_tcx.empty_parameter_environment()
-        });
+        let param_env = param_env.take().unwrap_or_else(|| ParameterEnvironment::empty());


We could just use ty::ParameterEnvironment here and avoid adding the import.

Speaking of which, renaming it to ParamEnv might be a good idea?

I always go back and forth on the importing question. I tend to prefer the import, so as to avoid having to write modules everywhere, and comply more closely with the Rust style guidelines (which suggest importing types, but not fns) -- but it does mean more merge conflicts around use declarations, which is annoying.

Mark-Simulacrum · 2017-05-15T20:09:46Z

src/librustc/ty/mod.rs

-        const FREEZENESS_CACHED = 1 << 20,
-        const IS_FREEZE         = 1 << 21,
-        const NEEDS_DROP_CACHED = 1 << 22,
-        const NEEDS_DROP        = 1 << 23,
    }


I think this is now small enough to be a u16 instead of a u32 bitflags!

It was always wasteful, heh. And I think due to padding it ends up using a lot anyway.

Mark-Simulacrum · 2017-05-15T20:13:09Z

src/librustc_trans/context.rs

    }

    pub fn type_is_freeze(&self, ty: Ty<'tcx>) -> bool {
-        ty.is_freeze(self.tcx, &self.empty_param_env, DUMMY_SP)
+        ty.is_freeze(self.tcx, ParameterEnvironment::empty(), DUMMY_SP)


Minor, but we could avoid the additional import with ty::ParamEnv. I think this occurs elsewhere, too.

eddyb · 2017-05-15T20:22:04Z

src/librustc/ty/mod.rs

    }
 }

 pub struct TyS<'tcx> {
    pub sty: TypeVariants<'tcx>,
-    pub flags: Cell<TypeFlags>,
+    pub flags: TypeFlags,


eddyb · 2017-05-15T20:24:46Z

src/librustc/ty/util.rs

+       .enter(|infcx| traits::type_known_to_meet_bound(&infcx, ty, trait_def_id, DUMMY_SP))
+}
+
+fn is_sized_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,


I see you killed some of the shortcutting?

I figured it'd fall out from the caching anyway.

eddyb

LGTM, modulo nits.

nikomatsakis · 2017-05-15T22:08:05Z

ok, I made some style changes (s/ParameterEnvironment/ParamEnv/, removed imports) -- I haven't finished building this locally yet though :)

Mark-Simulacrum · 2017-05-15T23:29:34Z

src/librustc/traits/README.md

@@ -418,7 +418,7 @@ before, and hence the cache lookup would succeed, yielding
 One subtle interaction is that the results of trait lookup will vary
 depending on what where clauses are in scope. Therefore, we actually
 have *two* caches, a local and a global cache. The local cache is
-attached to the `ParameterEnvironment` and the global cache attached
+attached to ParamEnv` and the global cache attached


This looks wrong, but is minor. Should ideally be "to the ParamEnv".

Mark-Simulacrum · 2017-05-15T23:32:47Z

src/librustc/ty/util.rs

@@ -16,7 +16,7 @@ use infer::InferCtxt;
 use ich::{StableHashingContext, NodeIdHashingMode};
 use traits::{self, Reveal};
 use ty::{self, Ty, TyCtxt, TypeFoldable};
-use ty::ParameterEnvironment;
+use ty::ParamEnv;


This is again somewhat painful to me since we use both ty::ParamEnv and ParamEnv in this file. Don't bother fixing if you don't want to though.

bors · 2017-05-18T00:37:02Z

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

carols10cents · 2017-05-22T16:47:57Z

@nikomatsakis looks like this is back in your court now, friendly ping!

Use the trait-environment+type as the key. Note that these are only invoked on types that live for the entire compilation (no inference artifacts). We no longer need the various special-case bits and caches that were in place before.

Ideally, we'd have the `Ty` inserted directly in the dep-node, but since we can't do that yet, we extract the characteristic def-id of the type in question.

nikomatsakis · 2017-05-22T21:23:46Z

@bors r=eddyb

bors · 2017-05-22T21:23:47Z

📌 Commit 83641a9 has been approved by eddyb

bors · 2017-05-23T07:14:48Z

⌛ Testing commit 83641a9 with merge 9fa25a7...

remove interior mutability of type-flags We were previously using the flags on `Ty<'tcx>` instances to do some ad-hoc caching schemes around things like `is_sized()`, `is_freeze()`, and `moves_by_default()`. This PR replaces those schemes with a proper query; the query key is based on the pair of a `(ParameterEnvironment<'tcx>, Ty<'tcx>)` pair. This is also intended to be a preliminary template for what trait-selection and projection will eventually look like. I did some performance measurements. In the past, I observed a noticeable speedup (6%) for building rustc, but since I've rebased, the numbers appear to be more of a wash: | Crate | Before | After | Percentage | | --- | --- | --- | -- | | syntax | 167s | 166s | 0.6% faster | | rustc | 376s | 382s | 1.5% slower | Some advantages of this new scheme: - `is_sized` etc are proper queries - we get caching across generic fns, so long as trait environment is identical - dependency tracking is correct

bors · 2017-05-23T09:55:35Z

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

rust-highfive assigned eddyb May 15, 2017

eddyb reviewed May 15, 2017

View reviewed changes

nikomatsakis force-pushed the chalk-trait-env-2 branch from 617ed25 to aa511ce Compare May 15, 2017 20:03

Mark-Simulacrum reviewed May 15, 2017

View reviewed changes

eddyb reviewed May 15, 2017

View reviewed changes

eddyb approved these changes May 15, 2017

View reviewed changes

Mark-Simulacrum reviewed May 15, 2017

View reviewed changes

nikomatsakis force-pushed the chalk-trait-env-2 branch from 4f431a4 to 3977ee7 Compare May 16, 2017 10:07

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2017

carols10cents 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 May 22, 2017

nikomatsakis added 12 commits May 22, 2017 14:55

make parameter-environment a query

194d4bc

refactor common logic into ParameterEnvironment::and()

5a5c265

move needs_drop into a query

b46c1a9

remove the cell from type flags completely

4464dcc

kill SIZEDNESS_CACHED

aa5232b

kill FREEZENESS_CACHED

25ea618

kill NEEDS_DROP_CACHED

ca65a8a

rename ParameterEnvironment to ParamEnv

226358e

use ParamEnv from ty rather than importing

697146b

rename parameter_environment to param_env

45dd063

two more style nits

9317d37

nikomatsakis force-pushed the chalk-trait-env-2 branch from e3435fb to 15abc19 Compare May 22, 2017 20:02

nikomatsakis added 2 commits May 22, 2017 17:11

fix DepNode

0436710

Ideally, we'd have the `Ty` inserted directly in the dep-node, but since we can't do that yet, we extract the characteristic def-id of the type in question.

fix atomic_lock_free test case

83641a9

nikomatsakis force-pushed the chalk-trait-env-2 branch from d9dab46 to 83641a9 Compare May 22, 2017 21:23

bors merged commit 83641a9 into rust-lang:master May 23, 2017

dwrensha mentioned this pull request May 24, 2017

update for upstream changes with ty::ParamEnv rust-lang/miri#170

Merged

est31 mentioned this pull request May 25, 2017

Regression from stable: "Unexpected tail in unsized_info_ty" #42210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove interior mutability of type-flags #42015

remove interior mutability of type-flags #42015

nikomatsakis commented May 15, 2017 •

edited

Loading

rust-highfive commented May 15, 2017

nikomatsakis commented May 15, 2017

eddyb May 15, 2017

Mark-Simulacrum left a comment

Mark-Simulacrum May 15, 2017

eddyb May 15, 2017

nikomatsakis May 15, 2017 •

edited

Loading

Mark-Simulacrum May 15, 2017

eddyb May 15, 2017

Mark-Simulacrum May 15, 2017

eddyb May 15, 2017

eddyb May 15, 2017

nikomatsakis May 15, 2017

eddyb left a comment

nikomatsakis commented May 15, 2017

Mark-Simulacrum May 15, 2017

nikomatsakis May 15, 2017

Mark-Simulacrum May 15, 2017

nikomatsakis May 15, 2017

bors commented May 18, 2017

carols10cents commented May 22, 2017

nikomatsakis commented May 22, 2017

bors commented May 22, 2017

bors commented May 23, 2017

bors commented May 23, 2017

remove interior mutability of type-flags #42015

remove interior mutability of type-flags #42015

Conversation

nikomatsakis commented May 15, 2017 • edited Loading

rust-highfive commented May 15, 2017

nikomatsakis commented May 15, 2017

Choose a reason for hiding this comment

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb left a comment

Choose a reason for hiding this comment

nikomatsakis commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 18, 2017

carols10cents commented May 22, 2017

nikomatsakis commented May 22, 2017

bors commented May 22, 2017

bors commented May 23, 2017

bors commented May 23, 2017

nikomatsakis commented May 15, 2017 •

edited

Loading

nikomatsakis May 15, 2017 •

edited

Loading