Add regression test for rope string slice performance (issue #26682)#26683
Add regression test for rope string slice performance (issue #26682)#26683
Conversation
String.prototype.slice() currently has O(n) complexity on rope strings, causing O(n²) overall complexity when iterating through a concatenated string with slice operations. The tests are marked as todo until the WebKit fix is merged: oven-sh/WebKit#154 Once the WebKit PR is merged and the commit hash is updated in cmake/tools/SetupWebKit.cmake, these tests should be enabled. Fixes #26682 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Updated 10:57 PM PT - Feb 1st, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 26683That installs a local version of the PR into your bun-26683 --bun |
WalkthroughAdds a regression test file containing three performance test cases for string operations on concatenated strings. The tests verify slice, charAt, and bracket access performance to detect quadratic versus linear time complexity behavior, referencing upstream WebKit fixes. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/regression/issue/026682.test.ts`:
- Around line 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.
- Around line 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.
- Around line 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.
| // Using 50,000 iterations - enough to detect O(n²) behavior | ||
| const iterations = 50000; | ||
| let s = ""; | ||
| for (let i = 0; i < iterations; i++) s += "A"; | ||
|
|
There was a problem hiding this comment.
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.
| // 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.
| 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; |
There was a problem hiding this comment.
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.
| 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.
| const iterations = 50000; | ||
| let s = ""; | ||
| for (let i = 0; i < iterations; i++) s += "A"; |
There was a problem hiding this comment.
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.
| 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.
1 participant
Summary
Fixes #26682
String.prototype.slice()currently has O(n) complexity on rope strings (strings built by concatenation), causing O(n²) overall complexity when iterating through a concatenated string with slice operations.Reproduction:
Results:
Root Cause
The issue is in JavaScriptCore's
tryJSSubstringImplfunction inJSString.h. When slicing across rope fiber boundaries, the implementation resolves the entire rope instead of extracting substrings from each fiber.Fix
The fix is in the WebKit fork: oven-sh/WebKit#154
The fix modifies
tryJSSubstringImplto handle cross-fiber slices by extracting substrings from each relevant fiber and concatenating them into a new rope, avoiding the need to resolve the entire original rope.This PR
This PR adds regression tests that are marked as
test.todo()until the WebKit fix is merged and the commit hash is updated incmake/tools/SetupWebKit.cmake.Test plan
USE_SYSTEM_BUN=1 bun test test/regression/issue/026682.test.tsWEBKIT_VERSIONincmake/tools/SetupWebKit.cmaketest.todo()totest()and verify tests pass🤖 Generated with Claude Code