mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #3467 from topecongiro/issue-3465

Browse Source 
Fix bad performance on deeply nested binary expressions
This commit is contained in:
Seiichi Uchida 2019-03-25 07:26:04 +09:00 committed by GitHub
commit 288d7db5a9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 139 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
.travis.yml
View File

@ -28,12 +28,13 @@ matrix:
- env: INTEGRATION=mdbook
- env: INTEGRATION=packed_simd
- env: INTEGRATION=rand
- env: INTEGRATION=rust-clippy
- env: INTEGRATION=rust-semverver
- env: INTEGRATION=stdsimd TARGET=x86_64-unknown-linux-gnu
- env: INTEGRATION=tempdir
- env: INTEGRATION=futures-rs
allow_failures:
# Doesn't build - keep this in allow_failures as it's fragile to breaking changes of rustc.
- env: INTEGRATION=rust-clippy
# Doesn't build - seems to be because of an option
- env: INTEGRATION=packed_simd
# Doesn't build - a temporal build failure due to breaking changes in the nightly compilre
@ -45,7 +46,7 @@ script:
- |
if [ -z ${INTEGRATION} ]; then
cargo build
cargo test
RUST_MIN_STACK=8388608 cargo test
else
./ci/integration.sh
fi

1
appveyor.yml
View File

@ -2,6 +2,7 @@
# and modified (mainly removal of deployment) to suit rustfmt.
environment:
RUST_MIN_STACK: 8388608
global:
PROJECT_NAME: rustfmt
matrix:

93
src/pairs.rs
View File

@ -34,19 +34,11 @@ pub(crate) fn rewrite_all_pairs(
shape: Shape,
context: &RewriteContext<'_>,
) -> Option<String> {
// First we try formatting on one line.
if let Some(list) = expr.flatten(false) {
if let Some(r) = rewrite_pairs_one_line(&list, shape, context) {
return Some(r);
}
}
// We can't format on line, so try many. When we flatten here we make sure
// to only flatten pairs with the same operator, that way we don't
// necessarily need one line per sub-expression, but we don't do anything
// too funny wrt precedence.
expr.flatten(true)
.and_then(|list| rewrite_pairs_multiline(&list, shape, context))
expr.flatten(context, shape).and_then(|list| {
// First we try formatting on one line.
rewrite_pairs_one_line(&list, shape, context)
.or_else(|| rewrite_pairs_multiline(&list, shape, context))
})
}
// This may return a multi-line result since we allow the last expression to go
@ -61,22 +53,23 @@ fn rewrite_pairs_one_line<T: Rewrite>(
let mut result = String::new();
let base_shape = shape.block();
for (e, s) in list.list.iter().zip(list.separators.iter()) {
let cur_shape = base_shape.offset_left(last_line_width(&result))?;
let rewrite = e.rewrite(context, cur_shape)?;
for ((_, rewrite), s) in list.list.iter().zip(list.separators.iter()) {
if let Some(rewrite) = rewrite {
if !is_single_line(&rewrite) || result.len() > shape.width {
return None;
}
if !is_single_line(&rewrite) || result.len() > shape.width {
result.push_str(&rewrite);
result.push(' ');
result.push_str(s);
result.push(' ');
} else {
return None;
}
result.push_str(&rewrite);
result.push(' ');
result.push_str(s);
result.push(' ');
}
let prefix_len = result.len();
let last = list.list.last().unwrap();
let last = list.list.last()?.0;
let cur_shape = base_shape.offset_left(last_line_width(&result))?;
let last_rewrite = last.rewrite(context, cur_shape)?;
result.push_str(&last_rewrite);
@ -112,10 +105,9 @@ fn rewrite_pairs_multiline<T: Rewrite>(
let indent_str = nested_shape.indent.to_string_with_newline(context.config);
let mut result = String::new();
let rewrite = list.list[0].rewrite(context, shape)?;
result.push_str(&rewrite);
result.push_str(&list.list[0].1.as_ref()?);
for (e, s) in list.list[1..].iter().zip(list.separators.iter()) {
for ((e, default_rw), s) in list.list[1..].iter().zip(list.separators.iter()) {
// The following test checks if we should keep two subexprs on the same
// line. We do this if not doing so would create an orphan and there is
// enough space to do so.
@ -139,24 +131,20 @@ fn rewrite_pairs_multiline<T: Rewrite>(
}
}
let nested_overhead = s.len() + 1;
let line_shape = match context.config.binop_separator() {
match context.config.binop_separator() {
SeparatorPlace::Back => {
result.push(' ');
result.push_str(s);
result.push_str(&indent_str);
nested_shape.sub_width(nested_overhead)?
}
SeparatorPlace::Front => {
result.push_str(&indent_str);
result.push_str(s);
result.push(' ');
nested_shape.offset_left(nested_overhead)?
}
};
}
let rewrite = e.rewrite(context, line_shape)?;
result.push_str(&rewrite);
result.push_str(&default_rw.as_ref()?);
}
Some(result)
}
@ -250,27 +238,46 @@ pub(crate) fn rewrite_pair<LHS, RHS>(
// A pair which forms a tree and can be flattened (e.g., binops).
trait FlattenPair: Rewrite + Sized {
// If `_same_op` is `true`, then we only combine binops with the same
// operator into the list. E.g,, if the source is `a * b + c`, if `_same_op`
// is true, we make `[(a * b), c]` if `_same_op` is false, we make
// `[a, b, c]`
fn flatten(&self, _same_op: bool) -> Option<PairList<'_, '_, Self>> {
fn flatten(&self, _: &RewriteContext<'_>, _: Shape) -> Option<PairList<'_, '_, Self>> {
None
}
}
struct PairList<'a, 'b, T: Rewrite> {
list: Vec<&'b T>,
list: Vec<(&'b T, Option<String>)>,
separators: Vec<&'a str>,
}
impl FlattenPair for ast::Expr {
fn flatten(&self, same_op: bool) -> Option<PairList<'_, '_, ast::Expr>> {
fn flatten(
&self,
context: &RewriteContext<'_>,
shape: Shape,
) -> Option<PairList<'_, '_, ast::Expr>> {
let top_op = match self.node {
ast::ExprKind::Binary(op, _, _) => op.node,
_ => return None,
};
let default_rewrite = |node: &ast::Expr, sep: usize, is_first: bool| {
if is_first {
return node.rewrite(context, shape);
}
let nested_overhead = sep + 1;
let rhs_offset = shape.rhs_overhead(&context.config);
let nested_shape = (match context.config.indent_style() {
IndentStyle::Visual => shape.visual_indent(0),
IndentStyle::Block => shape.block_indent(context.config.tab_spaces()),
})
.with_max_width(&context.config)
.sub_width(rhs_offset)?;
let default_shape = match context.config.binop_separator() {
SeparatorPlace::Back => nested_shape.sub_width(nested_overhead)?,
SeparatorPlace::Front => nested_shape.offset_left(nested_overhead)?,
};
node.rewrite(context, default_shape)
};
// Turn a tree of binop expressions into a list using a depth-first,
// in-order traversal.
let mut stack = vec![];
@ -279,12 +286,14 @@ fn flatten(&self, same_op: bool) -> Option<PairList<'_, '_, ast::Expr>> {
let mut node = self;
loop {
match node.node {
ast::ExprKind::Binary(op, ref lhs, _) if !same_op || op.node == top_op => {
ast::ExprKind::Binary(op, ref lhs, _) if op.node == top_op => {
stack.push(node);
node = lhs;
}
_ => {
list.push(node);
let op_len = separators.last().map_or(0, |s: &&str| s.len());
let rw = default_rewrite(node, op_len, list.is_empty());
list.push((node, rw));
if let Some(pop) = stack.pop() {
match pop.node {
ast::ExprKind::Binary(op, _, ref rhs) => {

42
tests/source/issue-3465.rs Normal file
View File

@ -0,0 +1,42 @@
fn main() {
((((((((((((((((((((((((((((((((((((((((((0) + 1) + 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1);
}

42
tests/target/issue-3465.rs Normal file
View File

@ -0,0 +1,42 @@
fn main() {
((((((((((((((((((((((((((((((((((((((((((0) + 1) + 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1)
+ 1);
}