-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(es/minifier): Implement partial evaluation of array join #10758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CodSpeed Performance ReportMerging #10758 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the minifier’s handling of Array.prototype.join
by introducing partial optimizations for mixed literal/expression arrays, adds empty-array join handling, and updates expected outputs and size snapshots throughout the test suite.
- Add
compress_array_join_partial
(and empty-array handling) inmisc.rs
to transform.join()
calls into more efficient concatenations. - Update numerous test fixtures and snapshot files to reflect new string-concatenation outputs and tiny size reductions.
- Introduce a
GroupType
enum to aid grouping logic in the new partial join optimization.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
crates/swc_ecma_minifier/src/compress/pure/misc.rs | Added empty-array join handling, compress_array_join_partial , and GroupType enum |
crates/swc_ecma_minifier/tests/terser/compress/arrays/constant_join_3/output.js | Updated expected output from .join() to explicit string concatenation |
crates/swc_ecma_minifier/tests/projects-size.snapshot.md | Adjusted compressed size for jquery.mobile-1.4.2.js |
crates/swc_ecma_minifier/tests/libs-size.snapshot.md | Minor size adjustments for echarts.js , terser.js , and three.js |
crates/swc_ecma_minifier/tests/fixture/** | Many fixture outputs updated to use string concatenations instead of .join() |
Comments suppressed due to low confidence (2)
crates/swc_ecma_minifier/src/compress/pure/misc.rs:99
- [nitpick] The enum variant names
Literals
andExpression
are somewhat ambiguous. Consider renaming them toLiteralGroup
andExpressionItem
(or similar) to clarify their roles.
enum GroupType<'a> {
crates/swc_ecma_minifier/src/compress/pure/misc.rs:702
- Add unit tests covering scenarios where: separators are longer than one character, comma separators with under six consecutive literals, and cases mixing literals and expressions, to verify both optimized and non-optimized paths.
// Only optimize for single-character separators to avoid bloating the code
@@ -625,6 +649,270 @@ impl Pure<'_> { | |||
.into() | |||
} | |||
|
|||
/// Performs partial optimization on array.join() when there are mixed literals and expressions. | |||
/// Groups consecutive literals into string concatenations. | |||
fn compress_array_join_partial( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This function exceeds 250 lines and handles multiple concerns (grouping, threshold logic, two build strategies). Consider extracting smaller helper functions (e.g., for grouping literals, building concatenations) to improve readability and testability.
Copilot uses AI. Check for mistakes.
if max_consecutive < 2 { | ||
return None; | ||
} | ||
|
||
// Only optimize for single-character separators to avoid bloating the code | ||
// Long separators like "really-long-separator" should not be optimized | ||
if separator.len() > 1 { | ||
return None; | ||
} | ||
|
||
// For comma separator, require a higher threshold to avoid infinite loops | ||
if separator == "," && max_consecutive < 6 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Thresholds like max_consecutive < 2
, separator.len() > 1
, and the comma-specific < 6
check are magic numbers. Extract them as named constants or document their rationale to make the code easier to understand and adjust.
if max_consecutive < 2 { | |
return None; | |
} | |
// Only optimize for single-character separators to avoid bloating the code | |
// Long separators like "really-long-separator" should not be optimized | |
if separator.len() > 1 { | |
return None; | |
} | |
// For comma separator, require a higher threshold to avoid infinite loops | |
if separator == "," && max_consecutive < 6 { | |
const MIN_CONSECUTIVE_LITERALS: usize = 2; | |
const MAX_CONSECUTIVE_FOR_COMMA: usize = 6; | |
const MAX_SEPARATOR_LENGTH: usize = 1; | |
if max_consecutive < MIN_CONSECUTIVE_LITERALS { | |
return None; | |
} | |
// Only optimize for single-character separators to avoid bloating the code | |
// Long separators like "really-long-separator" should not be optimized | |
if separator.len() > MAX_SEPARATOR_LENGTH { | |
return None; | |
} | |
// For comma separator, require a higher threshold to avoid infinite loops | |
if separator == "," && max_consecutive < MAX_CONSECUTIVE_FOR_COMMA { |
Copilot uses AI. Check for mistakes.
Description: