Perf regression in nightly when compiling massive matches

[swgillespie]

The file located here: https://gist.github.com/swgillespie/37b32f7b09ae536df8dc when compiled using rustc rustc_abuse.rs -o rustc_abuse -Z time-passes takes approximately 9 seconds and 40MB of memory to compile on stable and beta, while taking almost a minute and 4GB of memory on nightly.

The Bad Thing being done here is that there are several massive matches. As expected, match checking takes a few seconds, but the real culprit here seems to be "MIR dump", which is where the memory usage peaks. The memory usage is high enough to get my Travis CI build killed that has some code similar to this, but not as extreme.

These numbers were gathered with:

• rustc 1.5.0-nightly (4826f9625 2015-10-21)
• rustc 1.4.0-beta.3 (20eba406f 2015-10-16)
• rustc 1.3.0 (9a92aaf19 2015-09-15).
• Mac OS X 10.10.5

[alexcrichton]

cc @rust-lang/compiler, seems like some low hanging fruit perhaps?

[nikomatsakis]

Perhaps, yeah. Thanks for the report!

[nikomatsakis]

Well, it probably requires tweaking MIR somewhat to have a more general "switch" style statement, as @Aatch suggested at some point.

[eddyb]

@nikomatsakis I believe I've seen MIR building turn constant patterns not into plain x == constant but PartialEq::eq(&x, &constant) - wouldn't that be a bug, given that patterns need to be pure?

[nikomatsakis]

@eddyb ah yes, I meant to writeup an RFC on this point. I believe the
current pattern behavior (not what MIR does) is incorrect (or at least
undesirable). Certainly it's not something that I fully understood we were
doing. In particular, I think that

match v { CONSTANT => true, _ => false }

and

v == CONSTANT

should always be equivalent, if both of them compile. (I could imagine
that the match version might not always compile). The current behavior of
converting constants into patterns obviously doesn't preserve this
equivalence, which seems pretty wrong to me, but it also doesn't work very
well with things like associated constants.

On Fri, Oct 23, 2015 at 3:21 AM, Eduard-Mihai Burtescu <
notifications@github.com> wrote:

@nikomatsakis https://github.com/nikomatsakis I believe I've seen MIR
building turn constant patterns not into plain x == constant but PartialEq::eq(&x,
&constant) - wouldn't that be a bug, given that patterns need to be pure?

—
Reply to this email directly or view it on GitHub
#29227 (comment).

[eddyb]

I don't think there's a way to change that and not break backwards compat, as currently the purity of PartialEq impls is not publically reflected in APIs, and matching always works, even without a PartialEq impl or an impure one.

OTOH, expanding associated constants may require delaying match MIR generation post-monomorphization, at least for the associated constant pattern, not necessarily the whole match.

[nikomatsakis]

@eddyb I consider the current behavior a bug, that it is accepting more code than it ought to. But I would want to evaluate the impact of any change, to be sure. Note: I also don't consider purity in matches to be a requirement. After all, I'd like to permit `box` patterns, which do a deref. That is also note pure. We simply cannot expand associated constants, I don't think. I mean technically yes we could wait till after monomorphization, but we'd also have to avoid doing the exhaustiveness checks until that point as well, since they are influenced by the constant-to-pattern expansion.

[arielb1]

@nikomatsakis

I am not sure we want to support non-resolvable associated constants in match, exactly because of this unclarity. However, I am not sure match x { C => {}, _ => {}} into match x { a if a == C => {}, _ => {} } is so terrible.

[eddyb]

@nikomatsakis Exhaustivity-wise, we can just be conservative, which might indeed be too much of a restriction for associated constants to be useful at all in matches.

As for the box patterns, we need restrictions there for soundness, and I believe the best way to go about it is to reuse the DST support: the CoerceUnsized<Rc<U>> for Rc<T> impl lets the compiler know where the actual pointer is.
Coupled with a Deref<Target=T> for Rc<T> impl, the compiler can assume...

Actually, scratch that, it was for a different feature (self: Rc<T>) - the pointers the compiler knows about is to an RcBox<T>, not as useful here.
To get box patterns to work, we can just require an unsafe impl<T> PureDeref for Rc<T> {} and be done with it.
Although I wouldn't stabilize that because it could interfere with a potential const impl<T> Deref<Target=T> for Rc<T> {...}.

[nikomatsakis]

This is pretty off topic for this issue. I plan to open a discussion thread
in the next day or so and we can hash it out there. :) Alternatively,
#20489 has related discussion.

On Sat, Oct 24, 2015 at 3:50 AM, Eduard-Mihai Burtescu <
notifications@github.com> wrote:

@nikomatsakis https://github.com/nikomatsakis Exhaustivity-wise, we can
just be conservative, which might indeed be too much of a restriction for
associated constants to be useful at all in matches.

As for the box patterns, we need restrictions there for soundness, and I
believe the best way to go about it is to reuse the DST support: the CoerceUnsized<Rc>
for Rc impl lets the compiler know where the actual pointer is.
Coupled with a Deref<Target=T> for Rc impl, the compiler can assume...

Actually, scratch that, it was for a different feature (self: Rc) -
the pointers the compiler knows about is to an RcBox, not as useful
here.
To get box patterns to work, we can just require an unsafe impl
PureDeref for Rc {} and be done with it.
Although I wouldn't stabilize that because it could interfere with a
potential const impl Deref<Target=T> for Rc {...}.

—
Reply to this email directly or view it on GitHub
#29227 (comment).