Change substitutions from a flat vector into 3 vectors #14604

nikomatsakis · 2014-06-02T13:54:39Z

The current setup is to have a single vector of type parameters in
scope at any one time. We then have to concatenate the parameters from
the impl/trait with those of the method. This makes a lot of things
awkward, most notably associated fns ("static fns"). This branch
restructures the substitutions into three distinct namespaces (type,
self, fn). This makes most of the "type parameter management"
trivial. This also sets us up to support UFCS (though I haven't made
any particular changes in that direction in this patch).

Along the way, this patch fixes a few miscellaneous bits of code cleanup:

Patch resolve to detect references to out-of-scope type parameters,
rather than checking for "out of bound" indices during substitution
(fixes Resolve is not detecting out-of-scope type parameters #14603).
Move def out of libsyntax into librustc where it belongs. I should have
moved DefId too, but didn't.
Permit homogeneous tuples like (T, T, T) to be used as fixed-length
vectors like [T, ..3]. This is awfully handy, though public facing.
I suppose it requires an RFC.
Add some missing tests.

cc #5527

r? @pcwalton or @pnkfelix

alexcrichton · 2014-06-02T16:11:59Z

src/libcore/tuple.rs

+                fn mut_iter<'a>(&'a mut self) -> MutItems<'a, $T>;
+                fn get<'a>(&'a self, index: uint) -> &'a $T;
+                fn get_mut<'a>(&'a mut self, index: uint) -> &'a mut $T;
+            }


Should the get and get_mut methods return Option<&T> to push failure towards the client?

They should do whatever slices do. I think this has changed in the interim, so I can update.

I think the situation is a little confusing at the moment with a lack of Index traits. The slice method get returns an Option<&T>, and apparently get_mut doesn't exist on slices?!

For Vec, get and get_mut return &T and &mut T, but I believe this is a stopgap until the indexing traits are available, and then I think they should return the same thing as slices.

So basically we have two conventions, but I'd be more in favor of get and get_mut returning Option and then implementing the Index traits when they exist for tuples as well (failing if out of bounds).

On Mon, Jun 02, 2014 at 09:53:17AM -0700, Alex Crichton wrote:

So basically we have two conventions, but I'd be more in favor of
get and get_mut returning Option and then implementing the
Index traits when they exist for tuples as well (failing if out of
bounds).

Agreed, this maps to Python (and maybe other languages too?) and I
always found it reasonable there.

(i will wait until such an update before r-plusing this PR)

nikomatsakis · 2014-06-02T16:47:35Z

RFC for homogeneous tuple methods: rust-lang/rfcs#104

pnkfelix · 2014-06-03T11:58:15Z

src/librustc/middle/subst.rs

+/**
+ * A substitution mapping type/region parameters to new values. We
+ * identify each in-scope parameter by an *index* and a *parameter
+ * space* (which indices where the parameter is defined; see


typo: should be "which indicates"

pnkfelix · 2014-06-03T12:39:57Z

Looks good to me overall. If possible please try to fix the commit history where some things seem like they were accidentally squashed into the wrong commit.

Though @pcwalton may want to skim over the resolve changes, which I largely rubber-stamped.

nikomatsakis · 2014-06-03T18:31:39Z

Rebased. I removed the public-facing API changes and instead made them local to the compiler (see final commit). I also cleaned up the commit history. I will create a second PR with the homogeneous tuple API changes.

…b, r=pnkfelix Separate out initial refactorings for PR #14604

alexcrichton · 2014-06-10T17:27:14Z

Note that I have un-gated the builder that this PR failed on (#14791), so a rebase will likely allow this to land.

parameters This involves numerous substeps: 1. Treat Self same as any other parameter. 2. No longer compute offsets for method parameters. 3. Store all generic types (both trait/impl and method) with a method, eliminating odd discrepancies. 4. Stop doing unspeakable things to static methods and instead just use the natural types, now that we can easily add the type parameters from trait into the method's polytype. 5. No doubt some more. It was hard to separate these into distinct commits. Fixes rust-lang#13564

@pcwalton

… r=pcwalton The current setup is to have a single vector of type parameters in scope at any one time. We then have to concatenate the parameters from the impl/trait with those of the method. This makes a lot of things awkward, most notably associated fns ("static fns"). This branch restructures the substitutions into three distinct namespaces (type, self, fn). This makes most of the "type parameter management" trivial. This also sets us up to support UFCS (though I haven't made any particular changes in that direction in this patch). Along the way, this patch fixes a few miscellaneous bits of code cleanup: 1. Patch resolve to detect references to out-of-scope type parameters, rather than checking for "out of bound" indices during substitution (fixes #14603). 2. Move def out of libsyntax into librustc where it belongs. I should have moved DefId too, but didn't. 3. Permit homogeneous tuples like `(T, T, T)` to be used as fixed-length vectors like `[T, ..3]`. This is awfully handy, though public facing. I suppose it requires an RFC. 4. Add some missing tests. cc #5527 r? @pcwalton or @pnkfelix

huonw · 2014-07-03T01:47:32Z

It looks like this may've contributed to a 250MB jump in memory use.

pnkfelix · 2014-07-03T06:58:23Z

Hmm, I wonder if this is due to having triple the amount of excess capacity? Not sure if there's actually any correlation between the lengths of the three Vec's though.

We might just need to make calls to shrink_to_fit at certain points. Or change the representation of VecPerParamSpace so that it has just one vector "under the hood".

I will look into this today.

pnkfelix · 2014-07-03T20:52:50Z

As a quick hack tonight, I did try adding calls to shrink_to_fit (under the assumption that if I added such calls in the right places, that would serve as an approximate lower-bound on how much memory we could expect to save if we e.g. changed from 3 Vecs to a single Vec with some sort of start/end metadata for each range).

As far as I can tell, calling shrink_to_fit did not resolve the problem, in that when I compared commit 9153d8a against its predecessor (and observed 200M mem usage increase when compiling librustc), and then added the shrink_to_fit calls, the mem usage increase remained.

For reference, here is the patch I used, in case I missed something: pnkfelix@cf8e44e

pnkfelix · 2014-07-03T20:53:29Z

(There were a lot of other changes mixed in on this patch, so its possible that the 3 Vec part of the patch is a red herring here. Not sure yet.)

pcwalton · 2014-07-03T21:40:01Z

Note that shrink_to_fit() doesn't get rid of malloc slop.

In my informal measurements, this brings the peak memory usage when building librustc from 1662M down to 1502M. Since 1662 - 1502 = 160, this may not recover the entirety of the observed memory regression (250M) from PR rust-lang#14604. (However, according to my local measurements, the regression when building librustc was more like 209M, so perhaps this will still recover the lions share of the lost memory.)

…cwalton In my informal measurements, this brings the peak memory usage when building librustc from 1662M down to 1502M. Since 1662 - 1502 = 160, this may not recover the entirety of the observed memory regression (250M) from PR #14604. (However, according to my local measurements, the regression when building librustc was more like 209M, so perhaps this will still recover the lions share of the lost memory.)

nikomatsakis · 2014-07-07T12:05:27Z

I did at one point attempt an alternate version of this patch in which I used Rc<Vec<T>> in numerous places to try and get more re-use. This did not improve memory usage at all and in fact seemed to make things worse. Also, @cmr did some measurements of this patch before I landed it where things looked good -- but I can't recall if I compared against master, or only between the variation that used ref counting.

pnkfelix · 2014-07-07T16:25:15Z

@nikomatsakis Hmm interesting. The majority of the regression has at this point been fixed, as huon reports here #15418 (comment)

So, just to try to understand, in the alternative version that you mention, you were trying to identify instances of Vec that could be shared between distinct instances of VecPerParamSpace ?

nikomatsakis · 2014-07-07T18:51:18Z

On Mon, Jul 07, 2014 at 09:25:19AM -0700, Felix S Klock II wrote:

So, just to try to understand, in the alternative version that you
mention, you were trying to identify instances of Vec that could
be shared between distinct instances of VecPerParamSpace ?

Yes. Basically VecPerParamSpace stored Rc<Vec<T>>, and in some
places I used a Rc<VecPerParamSpace<T>> as well to avoid deep
cloning. This made the code a bit messier overall and didn't seam to
really help much in practice.

internal: Add minicore smoke test fix rust-lang#14501

alexcrichton reviewed Jun 2, 2014
View reviewed changes

nikomatsakis mentioned this pull request Jun 2, 2014

Proposal to add methods to homogeneous tuples ((T, T, ..., T)) so that they can be used as slices (&[T]) rust-lang/rfcs#104

Closed

pnkfelix reviewed Jun 3, 2014
View reviewed changes

edwardw mentioned this pull request Jun 4, 2014

Consider implementing "two-dimensional" index for ty::substs #13564

Closed

alexcrichton mentioned this pull request Jun 5, 2014

Dump results of analysis phase as CSV (DXR support in Rust) #13222

Merged

nikomatsakis mentioned this pull request Jun 6, 2014

Issue 5527 namespace substs b #14702

Merged

bors added a commit that referenced this pull request Jun 7, 2014

auto merge of #14702 : nikomatsakis/rust/issue-5527-namespace-substs-…

bd6683c

…b, r=pnkfelix Separate out initial refactorings for PR #14604

nikomatsakis mentioned this pull request Jun 13, 2014

Introduce VecPerParamSpace and use it to represent sets of types and parameters #14872

Closed

bors closed this Jun 13, 2014

bors merged commit 9153d8a into rust-lang:master Jun 13, 2014

pnkfelix mentioned this pull request Jul 4, 2014

Revise VecPerParamSpace to use a one Vec rather than three #15418

Merged

nikomatsakis deleted the issue-5527-namespace-substs branch March 30, 2016 16:15

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023

Auto merge of rust-lang#14604 - HKalbasi:dev3, r=Veykril

112464f

internal: Add minicore smoke test fix rust-lang#14501

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change substitutions from a flat vector into 3 vectors #14604

Change substitutions from a flat vector into 3 vectors #14604

nikomatsakis commented Jun 2, 2014

alexcrichton Jun 2, 2014

nikomatsakis Jun 2, 2014

alexcrichton Jun 2, 2014

nikomatsakis Jun 2, 2014

pnkfelix Jun 3, 2014

nikomatsakis commented Jun 2, 2014

pnkfelix Jun 3, 2014

pnkfelix commented Jun 3, 2014

nikomatsakis commented Jun 3, 2014

alexcrichton commented Jun 10, 2014

huonw commented Jul 3, 2014

pnkfelix commented Jul 3, 2014

pnkfelix commented Jul 3, 2014

pnkfelix commented Jul 3, 2014

pcwalton commented Jul 3, 2014

nikomatsakis commented Jul 7, 2014

pnkfelix commented Jul 7, 2014

nikomatsakis commented Jul 7, 2014

Change substitutions from a flat vector into 3 vectors #14604

Change substitutions from a flat vector into 3 vectors #14604

Conversation

nikomatsakis commented Jun 2, 2014

alexcrichton Jun 2, 2014

Choose a reason for hiding this comment

nikomatsakis Jun 2, 2014

Choose a reason for hiding this comment

alexcrichton Jun 2, 2014

Choose a reason for hiding this comment

nikomatsakis Jun 2, 2014

Choose a reason for hiding this comment

pnkfelix Jun 3, 2014

Choose a reason for hiding this comment

nikomatsakis commented Jun 2, 2014

pnkfelix Jun 3, 2014

Choose a reason for hiding this comment

pnkfelix commented Jun 3, 2014

nikomatsakis commented Jun 3, 2014

alexcrichton commented Jun 10, 2014

huonw commented Jul 3, 2014

pnkfelix commented Jul 3, 2014

pnkfelix commented Jul 3, 2014

pnkfelix commented Jul 3, 2014

pcwalton commented Jul 3, 2014

nikomatsakis commented Jul 7, 2014

pnkfelix commented Jul 7, 2014

nikomatsakis commented Jul 7, 2014