Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> #38097

Mark-Simulacrum · 2016-12-01T00:42:42Z

We refactor this in order to achieve the following wins:

Decrease the size of FnSig (Vec + bool: 32, &Slice + bool: 24).
Potentially decrease total allocated memory due to arena-allocating FnSig inputs/output; since they are allocated in the type list arena, other users of type lists can reuse the same allocation for an equivalent type list.
Remove the last part of the type system which needs drop glue (Refactor TraitObject to Slice<ExistentialPredicate> #37965 removed the other remaining part). This makes arenas containing FnSig faster to drop (since we don't need to drop a Vec for each one), and makes reusing them without clearing/dropping potentially possible.

eddyb · 2016-12-01T01:13:44Z

src/librustc/ty/relate.rs

-                      variadic: a.variadic})
+        let inputs_and_output = a.inputs().into_iter().cloned()
+            .zip(b.inputs().into_iter().cloned())
+            .zip(iter::repeat(false))


This can be done with .map.

eddyb · 2016-12-01T01:14:50Z

src/librustc/ty/structural_impls.rs

+            .map(|i| i.fold_with(folder))
+            .collect::<AccumulateVec<[_; 8]>>();
+        ty::FnSig {
+            inputs_and_output: folder.tcx().intern_type_list(&inputs_and_output),


I think you can just fold the list wholesale.

eddyb · 2016-12-01T01:16:32Z

src/librustc/ty/sty.rs

 pub type PolyFnSig<'tcx> = Binder<FnSig<'tcx>>;

 impl<'tcx> PolyFnSig<'tcx> {
-    pub fn inputs(&self) -> ty::Binder<Vec<Ty<'tcx>>> {
-        self.map_bound_ref(|fn_sig| fn_sig.inputs.clone())
+    pub fn inputs<'a>(&'a self) -> Binder<&[Ty<'tcx>]> {


The elision abuse here looks odd.

eddyb · 2016-12-01T01:17:27Z

src/librustc/ty/sty.rs

@@ -1243,10 +1252,6 @@ impl<'a, 'gcx, 'tcx> TyS<'tcx> {
    }

    // Type accessors for substructures of types
-    pub fn fn_args(&self) -> ty::Binder<Vec<Ty<'tcx>>> {
-        self.fn_sig().inputs()


You could keep this returning a slice.

eddyb · 2016-12-01T01:19:21Z

src/librustc_trans/callee.rs

-    };
+    let tuple_input_ty = tcx.intern_tup(sig.inputs());
+    let sig = tcx.mk_fn_sig(
+        iter::once(bare_fn_ty_maybe_ref).chain(iter::once(tuple_input_ty)),


You're using iter::once a bit too much - try arrays instead.

eddyb · 2016-12-01T01:20:07Z

src/librustc_trans/common.rs

-                ..sig
-            });
+            let sig = sig.map_bound(|sig| tcx.mk_fn_sig(
+                iter::once(env_ty).chain(sig.inputs().into_iter().cloned()),


Could you avoid .into_iter() on slices? It's just .iter().

eddyb · 2016-12-01T01:23:07Z

src/librustc_typeck/astconv.rs

@@ -1595,7 +1595,8 @@ impl<'o, 'gcx: 'tcx, 'tcx> AstConv<'gcx, 'tcx>+'o {
                // checking for here would be considered early bound
                // anyway.)
                let inputs = bare_fn_ty.sig.inputs();
-                let late_bound_in_args = tcx.collect_constrained_late_bound_regions(&inputs);
+                let late_bound_in_args = tcx.collect_constrained_late_bound_regions(
+                    &inputs.map_bound(|i| i.to_owned()));


Why would this be needed? Looks like a bug?

TypeFoldable isn't implemented for &[Ty<'tcx>], which is what we have, so we need to cast it to a vector in order to pass it here. This is rather unfortunate, but I'm not sure how much we can do. It may be worth pointing out that the use is a visit_with on it, so it's possible this can be refactored into passing either the slice or an iterator to solve this problem. I can look into that if you'd like.

KalitaAlexey · 2016-12-01T04:42:55Z

What is purpose of this?

Mark-Simulacrum · 2016-12-01T18:26:36Z

I believe there are a couple of purposes:

Decreasing the size of FnSig
- Vec + bool: 32, &Slice + bool: 24.
Potentially decrease total allocated memory due to arena-allocating FnSig inputs/output; since they are allocated in the type list arena, other users of type lists can reuse the same allocation for an equivalent type list.
Remove the last part of the type system which needs drop glue (Refactor TraitObject to Slice<ExistentialPredicate> #37965 removed the other remaining part)

@eddyb may be able to suggest other reasons.

@eddyb Ready for another review pass.

bors · 2016-12-02T18:21:22Z

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

Mark-Simulacrum · 2016-12-02T19:40:40Z

Rebased.

eddyb · 2016-12-02T22:04:14Z

src/librustc_typeck/lib.rs

-                }),
+                sig: ty::Binder(tcx.mk_fn_sig(
+                    iter::once(tcx.types.isize)
+                        .chain(iter::once(tcx.mk_imm_ptr(tcx.mk_imm_ptr(tcx.types.u8)))),


Could you use slice iterators, here and elsewhere?

eddyb · 2016-12-02T22:06:15Z

@Mark-Simulacrum Can you move the contents of that comment into the description? You may also want to mention that the drop glue thing is about arenas, both in terms of cost of dropping/reusing the temporary ones currently and experiments with unified arenas in the future.

eddyb · 2016-12-02T22:07:05Z

@rust-lang/compiler Any objections? This is ready to land otherwise, modulo a few "style" tweaks.

Mark-Simulacrum · 2016-12-02T22:27:13Z

Fixed style nit (I couldn't find any other places I chained iter::once together).

Added the comment for reasons why we want this to the description.

bors · 2016-12-06T03:47:11Z

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

Mark-Simulacrum · 2016-12-06T05:33:59Z

Rebased.

eddyb · 2016-12-06T05:38:09Z

@bors r+

bors · 2016-12-06T05:38:10Z

📌 Commit 296ec5f has been approved by eddyb

bors · 2016-12-06T07:30:23Z

⌛ Testing commit 296ec5f with merge ff261d3...

@eddyb

Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> We refactor this in order to achieve the following wins: - Decrease the size of `FnSig` (`Vec` + `bool`: 32, `&Slice` + `bool`: 24). - Potentially decrease total allocated memory due to arena-allocating `FnSig` inputs/output; since they are allocated in the type list arena, other users of type lists can reuse the same allocation for an equivalent type list. - Remove the last part of the type system which needs drop glue (#37965 removed the other remaining part). This makes arenas containing `FnSig` faster to drop (since we don't need to drop a Vec for each one), and makes reusing them without clearing/dropping potentially possible. r? @eddyb

bors · 2016-12-06T10:49:39Z

michaelwoerister · 2016-12-06T14:27:05Z

Nice!

rust-highfive assigned eddyb Dec 1, 2016

eddyb suggested changes Dec 1, 2016

View reviewed changes

Mark-Simulacrum force-pushed the fn-sig-slice branch from 6aee99f to 692dd7c Compare December 1, 2016 02:25

Mark-Simulacrum force-pushed the fn-sig-slice branch from 6bed7f6 to c0d3460 Compare December 1, 2016 18:17

Mark-Simulacrum force-pushed the fn-sig-slice branch from c0d3460 to 5a30bf0 Compare December 2, 2016 19:40

eddyb reviewed Dec 2, 2016

View reviewed changes

Mark-Simulacrum force-pushed the fn-sig-slice branch from 5a30bf0 to 2114a4b Compare December 2, 2016 22:17

Mark-Simulacrum added 2 commits December 5, 2016 22:22

Refactor ty::FnSig to privatize all fields

1eab19d

Refactor FnSig to contain a Slice for its inputs and outputs.

296ec5f

Mark-Simulacrum force-pushed the fn-sig-slice branch from 2114a4b to 296ec5f Compare December 6, 2016 05:33

eddyb approved these changes Dec 6, 2016

View reviewed changes

bors merged commit 296ec5f into rust-lang:master Dec 6, 2016

bors mentioned this pull request Dec 6, 2016

Add checkup for return statement outside of a function #37780

Merged

Arnavion mentioned this pull request Dec 11, 2016

WIP: rustup rust-lang/rust-clippy#1373

Merged

Mark-Simulacrum mentioned this pull request Dec 12, 2016

Cast suggestions #38099

Merged

Mark-Simulacrum deleted the fn-sig-slice branch December 27, 2016 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> #38097

Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> #38097

Mark-Simulacrum commented Dec 1, 2016 •

edited

Loading

eddyb Dec 1, 2016

eddyb Dec 1, 2016

eddyb Dec 1, 2016

eddyb Dec 1, 2016

eddyb Dec 1, 2016

eddyb Dec 1, 2016

eddyb Dec 1, 2016

Mark-Simulacrum Dec 1, 2016

KalitaAlexey commented Dec 1, 2016

Mark-Simulacrum commented Dec 1, 2016

bors commented Dec 2, 2016

Mark-Simulacrum commented Dec 2, 2016

eddyb Dec 2, 2016

eddyb commented Dec 2, 2016

eddyb commented Dec 2, 2016

Mark-Simulacrum commented Dec 2, 2016

bors commented Dec 6, 2016

Mark-Simulacrum commented Dec 6, 2016

eddyb commented Dec 6, 2016

bors commented Dec 6, 2016

bors commented Dec 6, 2016

bors commented Dec 6, 2016

michaelwoerister commented Dec 6, 2016

Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> #38097

Refactor ty::FnSig to contain a &'tcx Slice<Ty<'tcx>> #38097

Conversation

Mark-Simulacrum commented Dec 1, 2016 • 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

Choose a reason for hiding this comment

KalitaAlexey commented Dec 1, 2016

Mark-Simulacrum commented Dec 1, 2016

bors commented Dec 2, 2016

Mark-Simulacrum commented Dec 2, 2016

Choose a reason for hiding this comment

eddyb commented Dec 2, 2016

eddyb commented Dec 2, 2016

Mark-Simulacrum commented Dec 2, 2016

bors commented Dec 6, 2016

Mark-Simulacrum commented Dec 6, 2016

eddyb commented Dec 6, 2016

bors commented Dec 6, 2016

bors commented Dec 6, 2016

bors commented Dec 6, 2016

michaelwoerister commented Dec 6, 2016

Mark-Simulacrum commented Dec 1, 2016 •

edited

Loading