refactor(minifier): port esbuild's mangleStmts#8770
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
CodSpeed Performance ReportMerging #8770 will improve performances by 13.09%Comparing Summary
Benchmarks breakdown
|
d1d66e6 to
3dd592d
Compare
1581606 to
0b160b8
Compare
0b160b8 to
ac47e52
Compare
ac47e52 to
033b55f
Compare
|
@sapphi-red Please take a look, The previous google closure compiler architecture is not holding up well :-/ The current direction for performance is 1 AST pass with lots of small "minimize expression" calls when new expressions are created ... we'll get there! |
sapphi-red
left a comment
There was a problem hiding this comment.
Great! I left some comments
| Statement::ExpressionStatement(_) => { | ||
| if let Some(Statement::ReturnStatement(last_return)) = result.last() { | ||
| if last_return.argument.is_none() { | ||
| break 'return_loop; | ||
| } | ||
| } | ||
| self.mark_current_function_as_changed(); | ||
| // "a(); return b;" => "return a(), b;" | ||
| let last_stmt = result.pop().unwrap(); | ||
| let Statement::ReturnStatement(mut last_return) = last_stmt else { | ||
| unreachable!() | ||
| }; | ||
| let prev_stmt = result.pop().unwrap(); | ||
| let Statement::ExpressionStatement(mut expr_stmt) = prev_stmt else { | ||
| unreachable!() | ||
| }; | ||
| let b = last_return.argument.as_mut().unwrap(); | ||
| let argument = Self::join_sequence(&mut expr_stmt.expression, b, ctx); | ||
| let right_span = last_return.span; | ||
| let last_return_stmt = | ||
| ctx.ast.statement_return(right_span, Some(argument)); | ||
| result.push(last_return_stmt); | ||
| } |
There was a problem hiding this comment.
I guess this case is mostly handled by
oxc/crates/oxc_minifier/src/peephole/minimize_statements.rs
Lines 280 to 291 in 033b55f
When is this code needed?
| // Merge the last two statements | ||
| Statement::IfStatement(if_stmt) => { |
There was a problem hiding this comment.
Similar to the previous question, I wonder if this can be done inside self.minimize_statement by running self.minimize_statement when the last item of result changed.
| _ => {} | ||
| } | ||
| result.push(Statement::ForStatement(for_stmt)); | ||
| } |
There was a problem hiding this comment.
(I guess you're aware of it, but in case) I think ForInStatement, LabeledStatement, BlockStatement is missing here.
oxc/crates/oxc_minifier/src/peephole/statement_fusion.rs
Lines 70 to 80 in 29417dd
oxc/crates/oxc_minifier/src/peephole/statement_fusion.rs
Lines 134 to 150 in 29417dd
Maybe
BlockStatement should be handled in a different place though.
| _ => {} | ||
| } | ||
| result.push(Statement::ForStatement(for_stmt)); | ||
| } |
There was a problem hiding this comment.
(I guess you're aware of it, but in case) I think merging VariableDeclaration + ForInStatement/ForOfStatement is missing too.
oxc/crates/oxc_minifier/src/peephole/collapse_variable_declarations.rs
Lines 177 to 215 in 0fcff20
No description provided.