fix time complexity bug in string concat logic #10430

CodeMan62 · 2025-05-02T18:36:30Z

Description:
So there are some time complexity issues in string concat logic this PR tries to fix them. I have ensured that all tests are passing

BREAKING CHANGE:
N/A

Related issue (if exists):
#10219

changeset-bot · 2025-05-02T18:36:34Z

⚠️ No Changeset found

Latest commit: 766c409

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codspeed-hq · 2025-05-02T18:57:17Z

CodSpeed Performance Report

Merging #10430 will not alter performance

_{Comparing CodeMan62:fix/time-complexity (766c409) with main (3e99b8b)}

Summary

✅ 152 untouched benchmarks

kdy1

Cow::into_owned does not allocate in case it's not necessary.

crates/swc_ecma_minifier/src/compress/pure/strings.rs

kdy1 · 2025-05-03T14:48:13Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-            let s = lls.into_owned() + &*rls;
+            // Create a new string with capacity to avoid multiple allocations
+            let mut s = lls.into_owned() + &*rls;
+            s.reserve(rls.len());


This is inefficient because we already pushed the string

CodeMan62 · 2025-05-03T16:55:08Z

@kdy1 I think now this pr is good to go

kdy1 · 2025-05-06T20:54:27Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                        *cooked =
-                            format!("{}{}", convert_str_value_to_tpl_cooked(&ls.value), cooked)
-                                .into()
+                        let str_part = convert_str_value_to_tpl_cooked(&ls.value);


This allocates, so it's better to call str_part.reserve(cooked.len()); after this line and push the string to str_part

kdy1 · 2025-05-07T20:09:02Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                    r_first.raw = new;
+                    let raw_str_part = ls
+                        .raw
+                        .clone()


Suggested change

.clone()

kdy1 · 2025-05-07T20:09:24Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

+                        .map(|s| convert_str_raw_to_tpl_raw(&s[1..s.len() - 1]))
+                        .unwrap_or_else(|| convert_str_value_to_tpl_raw(&ls.value).into());
+
+                    let mut new_raw = String::with_capacity(raw_str_part.len() + r_first.raw.len());


Same here. You don't need to allocate a new string; rather you should append to raw_str_part

kdy1 · 2025-05-07T20:09:34Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

+                    // Calculate the total length to avoid multiple allocations
+                    let raw_str_part = rs
+                        .raw
+                        .clone()


Suggested change

.clone()

kdy1 · 2025-05-07T20:10:11Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                            format!("{}{}", cooked, convert_str_value_to_tpl_cooked(&rs.value))
-                                .into();
+                        // Create a new string with sufficient capacity
+                        let mut new_cooked = String::with_capacity(cooked.len() + rs.value.len());


You should not allocate a new string. You should rather append to cooked

Copilot

Pull Request Overview

This PR addresses performance issues in string concatenation logic by pre-allocating memory to reduce repeated string allocations.

Pre-allocates string buffers using estimated capacities instead of relying on default allocations.
Updates string concatenation patterns to use push operations for reduced overhead.

Comments suppressed due to low confidence (1)

crates/swc_ecma_minifier/src/compress/pure/strings.rs:48

Consider pre-allocating a new String with the combined capacity of 'lls' and 'rls' and using push_str operations rather than the '+' operator to avoid potential hidden reallocations.

let s = lls.into_owned() + &*rls;

crates/swc_ecma_minifier/src/compress/pure/strings.rs

CodeMan62 · 2025-05-13T04:56:32Z

I think now it is ready for review

kdy1 · 2025-05-13T14:25:19Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

@@ -391,20 +409,21 @@ impl Pure<'_> {
                    );

                    if let Some(cooked) = &mut l_last.cooked {
+                        // Direct concatenation with format! to avoid unnecessary allocations


This does not avoid unnecessary allocations

CodeMan62 · 2025-05-13T16:33:56Z

@kdy1 let me know if code is okay then I will fix fmt

kdy1 · 2025-05-14T16:19:37Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                                .into();
+                        let cooked_value = convert_str_value_to_tpl_cooked(&rs.value);
+                        // Pre-allocate additional space
+                        let mut owned_cooked = cooked.to_string();


This code is very inefficient. You don't need an allocation here.

CodeMan62 · 2025-05-14T18:35:02Z

@kdy1 I request you to help me further in this I can't figure out how to finish it I think the PR is almost done just append to raw_str_part remains help me in that 🙏

fixed

CodeMan62 · 2025-05-16T05:05:57Z

Everything is done now ready for review

CodeMan62 · 2025-05-17T15:31:10Z

@kdy1 general ping for review, let me know if still things left??

kdy1 · 2025-05-17T19:43:56Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                            format!("{}{}", cooked, convert_str_value_to_tpl_cooked(&rs.value))
-                                .into();
+                        let mut str_part = convert_str_value_to_tpl_cooked(&rs.value).into_owned();
+                        str_part.reserve(cooked.len());


This line is not required

kdy1 · 2025-05-17T19:44:05Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

+                    // Calculate the total length to avoid multiple allocations
+                    let raw_str_part = rs
+                        .raw
+                        .clone()


kdy1 · 2025-05-17T19:44:22Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                            format!("{}{}", convert_str_value_to_tpl_cooked(&ls.value), cooked)
-                                .into()
+                        let mut str_part = convert_str_value_to_tpl_cooked(&ls.value).into_owned();
+                        str_part.reserve(cooked.len());


Not required

kdy1 · 2025-05-17T19:45:16Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                    r_first.raw = new;
+                    // Convert to owned string, reserve space and append
+                    let mut owned_raw = l_last.raw.to_string();
+                    owned_raw.reserve(r_first.raw.len());


Not required

CodeMan62 · 2025-05-19T16:03:17Z

@kdy1 everything is done as you said review required

kdy1 · 2025-05-19T16:06:01Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

+                        let mut str_part = convert_str_value_to_tpl_cooked(&rs.value).into_owned();
+                        str_part.push_str(cooked);
+                        *cooked = swc_atoms::Atom::from(str_part);


This changes behavior (wrongly), allocates more

kdy1 · 2025-05-19T16:07:57Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

+                    // Calculate the total length to avoid multiple allocations
+                    let raw_str_part = match &rs.raw {
+                        Some(s) => convert_str_raw_to_tpl_raw(&s[1..s.len() - 1]),
+                        None => convert_str_value_to_tpl_raw(&rs.value).into(),


This seems inefficient. You don't need String here. Use Cow instead

kdy1 · 2025-05-19T16:08:21Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs


+                    let mut raw = raw_str_part.to_string();


This is super inefficient

kdy1 · 2025-05-19T16:08:42Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

+                let additional_quasis = rt.quasis.len();
+                let additional_exprs = rt.exprs.len();
+
+                l.quasis.reserve(additional_quasis);


This is not required

kdy1 · 2025-05-19T16:08:46Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

+                let additional_exprs = rt.exprs.len();
+
+                l.quasis.reserve(additional_quasis);
+                l.exprs.reserve(additional_exprs);


kdy1 · 2025-05-19T16:08:55Z

crates/swc_ecma_minifier/src/compress/pure/strings.rs

-                                let new_str = format!("{second_str}{third_str}");
+                                // Directly create a new string with the combined content
+                                let mut second_owned = second_str.into_owned();
+                                second_owned.reserve(third_str.len());


This is not required

CodeMan62 · 2025-05-19T18:34:16Z

@kdy1 thanks for all your reviews appreciation but I want to say sorry I am tired trying things here in this PR so I request you to help me fix this maybe I am missing something 🙏

fix time complexity bug in string concat logic

bc1e82a

CodeMan62 requested a review from a team as a code owner May 2, 2025 18:36

CodeMan62 mentioned this pull request May 2, 2025

minify.compress may cause memory overflow? #10219

Open

fmt fix

c3060a0

kdy1 requested changes May 2, 2025

View reviewed changes

crates/swc_ecma_minifier/src/compress/pure/strings.rs Show resolved Hide resolved

CodeMan62 added 2 commits May 3, 2025 15:26

resolve conversation

6c599df

test fix

efcedd2

CodeMan62 mentioned this pull request May 3, 2025

Missing variable after compression #10412

Closed

kdy1 reviewed May 3, 2025

View reviewed changes

refactor

f560288

kdy1 self-assigned this May 3, 2025

kdy1 added this to the Planned milestone May 3, 2025

kdy1 reviewed May 6, 2025

View reviewed changes

resolve conversation

a6a3ea4

kdy1 requested changes May 7, 2025

View reviewed changes

CodeMan62 added 3 commits May 11, 2025 17:50

append to raw_str_part

b0501a6

revert last changes

db1c9a4

fmt fix

6ff3958

CodeMan62 force-pushed the fix/time-complexity branch from 3cc2978 to 6ff3958 Compare May 11, 2025 18:43

kdy1 requested a review from Copilot May 11, 2025 23:11

Copilot AI reviewed May 11, 2025

View reviewed changes

crates/swc_ecma_minifier/src/compress/pure/strings.rs Outdated Show resolved Hide resolved

crates/swc_ecma_minifier/src/compress/pure/strings.rs Outdated Show resolved Hide resolved

kdy1 marked this pull request as draft May 11, 2025 23:13

resolve allocation

9845fef

CodeMan62 force-pushed the fix/time-complexity branch from 9124e35 to 9845fef Compare May 12, 2025 19:17

CodeMan62 added 2 commits May 13, 2025 00:53

Merge branch 'main' into fix/time-complexity

f37a02d

replace hard coded value with constant

d72dd58

CodeMan62 marked this pull request as ready for review May 13, 2025 04:56

kdy1 requested changes May 13, 2025

View reviewed changes

kdy1 marked this pull request as draft May 13, 2025 14:25

refactor allocation

0efdcca

fmt fix

9686fcf

CodeMan62 force-pushed the fix/time-complexity branch from 80ecd02 to 9686fcf Compare May 14, 2025 16:01

kdy1 reviewed May 14, 2025

View reviewed changes

resolve allocation of cooked

0e619fe

CodeMan62 force-pushed the fix/time-complexity branch from 1d75b41 to 0e619fe Compare May 14, 2025 18:33

final refactor

c24ae7a

CodeMan62 force-pushed the fix/time-complexity branch from 6b5a066 to c24ae7a Compare May 16, 2025 05:05

CodeMan62 marked this pull request as ready for review May 16, 2025 05:06

Merge branch 'main' into fix/time-complexity

4e93cc3

kdy1 requested changes May 17, 2025

View reviewed changes

remove unrequired things and fix rs variable

cf0b44e

Merge branch 'main' into fix/time-complexity

766c409

kdy1 requested changes May 19, 2025

View reviewed changes

Uh oh!

fix time complexity bug in string concat logic #10430

Are you sure you want to change the base?

fix time complexity bug in string concat logic #10430

Uh oh!

Conversation

CodeMan62 commented May 2, 2025

Uh oh!

changeset-bot bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

codspeed-hq bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #10430 will not alter performance

Summary

Uh oh!

kdy1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeMan62 commented May 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

CodeMan62 commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeMan62 commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeMan62 commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodeMan62 commented May 16, 2025

Uh oh!

CodeMan62 commented May 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeMan62 commented May 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changeset-bot bot commented May 2, 2025 •

edited

Loading

codspeed-hq bot commented May 2, 2025 •

edited

Loading

CodeMan62 commented May 14, 2025 •

edited

Loading