Skip to content

Commit

Permalink
Never expand specialized columns that only contain wild patterns in them
Browse files Browse the repository at this point in the history
Doing so would incur deeply nested expansion of the tree with no useful
side effects. This is problematic for "wide" data types such as structs
with dozens of fields but where only a few are actually being matched or bound.
Most notably, matching a fixed slice would use a number of stack frames that
grows with the number of elements in the slice.

Fixes #17877.
  • Loading branch information
Jakub Wieczorek committed Oct 12, 2014
1 parent 86509d8 commit 0c48c57
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 62 deletions.
4 changes: 2 additions & 2 deletions src/librustc/middle/check_match.rs
Expand Up @@ -638,15 +638,15 @@ fn pat_constructors(cx: &MatchCheckCtxt, p: &Pat,
PatEnum(..) =>
match cx.tcx.def_map.borrow().find(&pat.id) {
Some(&DefConst(..)) =>
cx.tcx.sess.span_bug(pat.span, "static pattern should've \
cx.tcx.sess.span_bug(pat.span, "const pattern should've \
been rewritten"),
Some(&DefVariant(_, id, _)) => vec!(Variant(id)),
_ => vec!(Single)
},
PatStruct(..) =>
match cx.tcx.def_map.borrow().find(&pat.id) {
Some(&DefConst(..)) =>
cx.tcx.sess.span_bug(pat.span, "static pattern should've \
cx.tcx.sess.span_bug(pat.span, "const pattern should've \
been rewritten"),
Some(&DefVariant(_, id, _)) => vec!(Variant(id)),
_ => vec!(Single)
Expand Down
16 changes: 15 additions & 1 deletion src/librustc/middle/pat_util.rs
Expand Up @@ -25,11 +25,25 @@ pub type PatIdMap = HashMap<Ident, NodeId>;
pub fn pat_id_map(dm: &resolve::DefMap, pat: &Pat) -> PatIdMap {
let mut map = HashMap::new();
pat_bindings(dm, pat, |_bm, p_id, _s, path1| {
map.insert(path1.node, p_id);
map.insert(path1.node, p_id);
});
map
}

pub fn pat_is_refutable(dm: &resolve::DefMap, pat: &Pat) -> bool {
match pat.node {
PatLit(_) | PatRange(_, _) => true,
PatEnum(_, _) | PatIdent(_, _, None) | PatStruct(..) => {
match dm.borrow().find(&pat.id) {
Some(&DefVariant(..)) => true,
_ => false
}
}
PatVec(_, _, _) => true,
_ => false
}
}

pub fn pat_is_variant_or_struct(dm: &resolve::DefMap, pat: &Pat) -> bool {
match pat.node {
PatEnum(_, _) | PatIdent(_, _, None) | PatStruct(..) => {
Expand Down
128 changes: 69 additions & 59 deletions src/librustc/middle/trans/_match.rs
Expand Up @@ -218,6 +218,7 @@ use util::ppaux::{Repr, vec_map_to_string};

use std;
use std::collections::HashMap;
use std::iter::AdditiveIterator;
use std::rc::Rc;
use syntax::ast;
use syntax::ast::{DUMMY_NODE_ID, Ident};
Expand Down Expand Up @@ -754,33 +755,41 @@ impl FailureHandler {
}
}

fn pick_col(m: &[Match]) -> uint {
fn score(p: &ast::Pat) -> uint {
match p.node {
ast::PatLit(_) | ast::PatEnum(_, _) | ast::PatRange(_, _) => 1u,
ast::PatIdent(_, _, Some(ref p)) => score(&**p),
_ => 0u
fn pick_column_to_specialize(def_map: &DefMap, m: &[Match]) -> Option<uint> {
fn pat_score(def_map: &DefMap, pat: &ast::Pat) -> uint {
match pat.node {
ast::PatIdent(_, _, Some(ref inner)) => pat_score(def_map, &**inner),
_ if pat_is_refutable(def_map, pat) => 1u,
_ => 0u
}
}
let mut scores = Vec::from_elem(m[0].pats.len(), 0u);
for br in m.iter() {
for (i, ref p) in br.pats.iter().enumerate() {
*scores.get_mut(i) += score(&***p);

let column_score: |&[Match], uint| -> uint = |m, col| {
let total_score = m.iter()
.map(|row| row.pats[col])
.map(|pat| pat_score(def_map, pat))
.sum();

// Irrefutable columns always go first, they'd only be duplicated in the branches.
if total_score == 0 {
std::uint::MAX
} else {
total_score
}
}
let mut max_score = 0u;
let mut best_col = 0u;
for (i, score) in scores.iter().enumerate() {
let score = *score;

// Irrefutable columns always go first, they'd only be duplicated in
// the branches.
if score == 0u { return i; }
// If no irrefutable ones are found, we pick the one with the biggest
// branching factor.
if score > max_score { max_score = score; best_col = i; }
}
return best_col;
};

let column_contains_any_nonwild_patterns: |&uint| -> bool = |&col| {
m.iter().any(|row| match row.pats[col].node {
ast::PatWild(_) => false,
_ => true
})
};

range(0, m[0].pats.len())
.filter(column_contains_any_nonwild_patterns)
.map(|col| (col, column_score(m, col)))
.max_by(|&(_, score)| score)
.map(|(col, _)| col)
}

// Compiles a comparison between two things.
Expand Down Expand Up @@ -951,44 +960,45 @@ fn compile_submatch<'a, 'p, 'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
return;
}

let col_count = m[0].pats.len();
if col_count == 0u {
let data = &m[0].data;
for &(ref ident, ref value_ptr) in m[0].bound_ptrs.iter() {
let llmatch = data.bindings_map.get(ident).llmatch;
call_lifetime_start(bcx, llmatch);
Store(bcx, *value_ptr, llmatch);
let tcx = bcx.tcx();
let def_map = &tcx.def_map;
match pick_column_to_specialize(def_map, m) {
Some(col) => {
let val = vals[col];
if has_nested_bindings(m, col) {
let expanded = expand_nested_bindings(bcx, m, col, val);
compile_submatch_continue(bcx,
expanded.as_slice(),
vals,
chk,
col,
val,
has_genuine_default)
} else {
compile_submatch_continue(bcx, m, vals, chk, col, val, has_genuine_default)
}
}
match data.arm.guard {
Some(ref guard_expr) => {
bcx = compile_guard(bcx,
&**guard_expr,
m[0].data,
m[1..m.len()],
vals,
chk,
has_genuine_default);
None => {
let data = &m[0].data;
for &(ref ident, ref value_ptr) in m[0].bound_ptrs.iter() {
let llmatch = data.bindings_map.get(ident).llmatch;
call_lifetime_start(bcx, llmatch);
Store(bcx, *value_ptr, llmatch);
}
_ => ()
match data.arm.guard {
Some(ref guard_expr) => {
bcx = compile_guard(bcx,
&**guard_expr,
m[0].data,
m[1..m.len()],
vals,
chk,
has_genuine_default);
}
_ => ()
}
Br(bcx, data.bodycx.llbb);
}
Br(bcx, data.bodycx.llbb);
return;
}

let col = pick_col(m);
let val = vals[col];

if has_nested_bindings(m, col) {
let expanded = expand_nested_bindings(bcx, m, col, val);
compile_submatch_continue(bcx,
expanded.as_slice(),
vals,
chk,
col,
val,
has_genuine_default)
} else {
compile_submatch_continue(bcx, m, vals, chk, col, val, has_genuine_default)
}
}

Expand Down
21 changes: 21 additions & 0 deletions src/test/run-pass/issue-17877.rs
@@ -0,0 +1,21 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn main() {
assert_eq!(match [0u8, ..1024] {
_ => 42u,
}, 42u);

assert_eq!(match [0u8, ..1024] {
[1, _..] => 0u,
[0, _..] => 1u,
_ => 2u
}, 1u);
}

5 comments on commit 0c48c57

@bors
Copy link
Contributor

@bors bors commented on 0c48c57 Oct 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bors
Copy link
Contributor

@bors bors commented on 0c48c57 Oct 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging jakub-/rust/issue-17877 = 0c48c57 into auto

@bors
Copy link
Contributor

@bors bors commented on 0c48c57 Oct 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jakub-/rust/issue-17877 = 0c48c57 merged ok, testing candidate = ff0abf0

@bors
Copy link
Contributor

@bors bors commented on 0c48c57 Oct 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bors
Copy link
Contributor

@bors bors commented on 0c48c57 Oct 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fast-forwarding master to auto = ff0abf0

Please sign in to comment.