Skip to content

Add regression test for rope string slice performance (issue #26682) #26683

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.

Sign up for GitHub

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

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add regression test for rope string slice performance (issue #26682) #26683

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions test/regression/issue/026682.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { expect, test } from "bun:test";

// Test for https://github.com/oven-sh/bun/issues/26682
// String.prototype.slice() should have O(1) complexity on rope strings,
// not O(n) which causes O(n²) overall when iterating through a concatenated string.
//
// The fix for this issue is in the WebKit fork: https://github.com/oven-sh/WebKit/pull/154
// These tests are marked as todo until the WebKit fix is merged and the commit hash is updated.

test.todo("rope string slice should be efficient (not O(n²))", () => {
// Create a rope string by concatenation
// Using 50,000 iterations - enough to detect O(n²) behavior
const iterations = 50000;
let s = "";
for (let i = 0; i < iterations; i++) s += "A";

Comment on lines +12 to +16
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use Buffer.alloc(...).toString() for repetitive strings (avoid char-by-char concat).

The current loop builds a repetitive string one character at a time. Use chunked Buffer.alloc to comply with test guidelines and avoid O(n²) setup costs while still creating a rope via concatenation.

Suggested update 
-  const iterations = 50000;
-  let s = "";
-  for (let i = 0; i < iterations; i++) s += "A";
+  const iterations = 50000;
+  const chunkSize = 1000;
+  const chunk = Buffer.alloc(chunkSize, "A").toString();
+  let s = "";
+  for (let i = 0; i < iterations; i += chunkSize) s += chunk;

As per coding guidelines: "Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Using 50,000 iterations - enough to detect O(n²) behavior
const iterations = 50000;
let s = "";
for (let i = 0; i < iterations; i++) s += "A";
// Using 50,000 iterations - enough to detect O(n²) behavior
const iterations = 50000;
const chunkSize = 1000;
const chunk = Buffer.alloc(chunkSize, "A").toString();
let s = "";
for (let i = 0; i < iterations; i += chunkSize) s += chunk;
🤖 Prompt for AI Agents 
In `@test/regression/issue/026682.test.ts` around lines 12 - 16, The test
currently builds a long repetitive string by appending one character at a time
using the iterations variable and s string (for loop that does s += "A"), which
is O(n²); replace that loop by creating the string in one allocation using
Buffer.alloc(iterations, "A").toString() and assign it to s so the test still
produces a long rope but avoids char-by-char concatenation.

// Test slice performance
const start = performance.now();
const m = new Map();
for (let i = 0; i < iterations; i++) {
const k = s.slice(i, i + 1);
m.set(k, 1);
}
const elapsed = performance.now() - start;

// With O(n²) complexity, this would take several seconds (>5000ms)
// With O(n) complexity, this should complete in under 500ms
// We use a generous threshold to avoid flakiness while still catching the regression
expect(elapsed).toBeLessThan(2000);
});

test.todo("rope string slice across multiple fibers should be efficient", () => {
// Create a rope string with multiple concatenations to ensure multiple fibers
const chunkSize = 10000;
let s = "";
for (let i = 0; i < 5; i++) {
let chunk = "";
for (let j = 0; j < chunkSize; j++) {
chunk += String.fromCharCode(65 + i); // A, B, C, D, E
}
s += chunk;
Comment on lines +34 to +41
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Create chunks via Buffer.alloc instead of per-char concatenation.

This inner loop is building a repetitive chunk one character at a time. Use Buffer.alloc(chunkSize, fill).toString() and keep the rope by concatenating chunks.

Suggested update 
-  const chunkSize = 10000;
-  let s = "";
-  for (let i = 0; i < 5; i++) {
-    let chunk = "";
-    for (let j = 0; j < chunkSize; j++) {
-      chunk += String.fromCharCode(65 + i); // A, B, C, D, E
-    }
-    s += chunk;
-  }
+  const chunkSize = 10000;
+  let s = "";
+  for (let i = 0; i < 5; i++) {
+    const chunk = Buffer.alloc(chunkSize, String.fromCharCode(65 + i)).toString(); // A, B, C, D, E
+    s += chunk;
+  }

As per coding guidelines: "Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const chunkSize = 10000;
let s = "";
for (let i = 0; i < 5; i++) {
let chunk = "";
for (let j = 0; j < chunkSize; j++) {
chunk += String.fromCharCode(65 + i); // A, B, C, D, E
}
s += chunk;
const chunkSize = 10000;
let s = "";
for (let i = 0; i < 5; i++) {
const chunk = Buffer.alloc(chunkSize, String.fromCharCode(65 + i)).toString(); // A, B, C, D, E
s += chunk;
}
🤖 Prompt for AI Agents 
In `@test/regression/issue/026682.test.ts` around lines 34 - 41, The inner loop
builds each chunk one character at a time causing inefficiency; replace the
per-char concatenation in the test (the variables chunk and the inner for-loop
that creates chunk using String.fromCharCode) with a single allocation using
Buffer.alloc(chunkSize, fillChar).toString() (where fillChar is
String.fromCharCode(65 + i)), then append that chunk to s as before to preserve
the rope behavior.

}

// Test slices that cross fiber boundaries
const start = performance.now();
const slices: string[] = [];

// Take slices near fiber boundaries (every 10000 chars)
for (let i = 0; i < s.length - 100; i += 1000) {
slices.push(s.slice(i, i + 100));
}
const elapsed = performance.now() - start;

// Verify correctness
expect(slices.length).toBeGreaterThan(0);
expect(slices[0].length).toBe(100);

// Performance check - should be fast
expect(elapsed).toBeLessThan(1000);
});

test("rope string charAt and bracket notation should be efficient", () => {
// This test verifies that charAt and bracket notation remain efficient
// (they were not affected by the bug, but we should ensure they stay fast)
const iterations = 50000;
let s = "";
for (let i = 0; i < iterations; i++) s += "A";
Comment on lines +65 to +67
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use chunked Buffer.alloc for the repetitive rope string.

Build the rope by concatenating chunks created via Buffer.alloc to comply with test guidelines and keep setup cost predictable.

Suggested update 
-  const iterations = 50000;
-  let s = "";
-  for (let i = 0; i < iterations; i++) s += "A";
+  const iterations = 50000;
+  const chunkSize = 1000;
+  const chunk = Buffer.alloc(chunkSize, "A").toString();
+  let s = "";
+  for (let i = 0; i < iterations; i += chunkSize) s += chunk;

As per coding guidelines: "Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const iterations = 50000;
let s = "";
for (let i = 0; i < iterations; i++) s += "A";
const iterations = 50000;
const chunkSize = 1000;
const chunk = Buffer.alloc(chunkSize, "A").toString();
let s = "";
for (let i = 0; i < iterations; i += chunkSize) s += chunk;
🤖 Prompt for AI Agents 
In `@test/regression/issue/026682.test.ts` around lines 65 - 67, Replace the
manual loop that builds the repetitive rope string (variables iterations and s)
with a chunked construction using Buffer.alloc(count, 'A').toString(): choose a
chunkSize (e.g., 1024), compute fullChunks = Math.floor(iterations / chunkSize)
and remainder = iterations % chunkSize, create the chunk string once via
Buffer.alloc(chunkSize, 'A').toString(), push that chunk fullChunks times (and a
final Buffer.alloc(remainder, 'A').toString() when remainder > 0) into an array,
then join to produce s; this keeps setup cost predictable and follows the
guideline to use Buffer.alloc instead of repeated concatenation or 'A'.repeat.


const start = performance.now();
let count = 0;
for (let i = 0; i < iterations; i++) {
if (s[i] === "A") count++;
}
const elapsed = performance.now() - start;

expect(count).toBe(iterations);
expect(elapsed).toBeLessThan(500);
});