Skip to content

Conversation

@kyle-apex
Copy link
Contributor

@kyle-apex kyle-apex commented Apr 5, 2025

Context

I was trying to reproduce the "Blank chat window of death" issue by examining bottle necks for a huge chat history.

combineApiRequests was where it became unresponsive for my 200MB - 600m token test.

With this update, my unrealistic task history data loads properly, so I can now properly reproduce the Blank chat window of death if I drag the scrollbar too high and virtuoso freaks out. (the good news is that basic scrolling still worked well)

It's not the full fix for the unresponsive issue (Which I believe it contributed to by the bottom setting in Virtuoso for large tasks), but it's an improvement.

Implementation

I had Roo write up some tests for the original combineApiRequests, then write a new, better performing version of the function. (And reviewed of course)

Screenshots

Because I used Claude, it went wild and created some simple benchmarks:
For 100 API Requests: 1.45x faster
For 500 API Requests: 2.14x faster
For 1,000 API Requests: 3.44x faster
For 100,000 API Requests: 37x faster (this is essentially what my test task history contained)

How to Test

Load any task from task history


Important

Refactor combineApiRequests to improve performance and add comprehensive tests for various cases.

  • Performance Improvement:
    • Refactor combineApiRequests in combineApiRequests.ts to improve performance from O(n^2) to O(n).
    • Uses a single pass with a stack to track api_req_started messages and combine them with api_req_finished messages.
  • Behavior:
    • Combines api_req_started and api_req_finished messages into a single message with merged JSON data.
    • Preserves non-API messages and handles interleaved API and non-API messages.
    • Filters out api_req_finished messages from the result.
  • Testing:
    • Adds comprehensive tests in combineApiRequests.test.ts to verify basic functionality, edge cases, and bug fixes.
    • Tests include handling of empty arrays, missing text fields, multiple matches, and additional properties.

This description was created by Ellipsis for 1418823. It will automatically update as commits are pushed.

@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2025

⚠️ No Changeset found

Latest commit: dec559b

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Apr 5, 2025
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 5, 2025
@cte
Copy link
Collaborator

cte commented Apr 5, 2025

Thanks so much! I verified that this significantly reduces the number of gray screens using our new evals system. Here's a benchmark script and the results as well:

// npx tsx optimizations/benchmarks/combineApiRequests-benchmark.ts

import { ClineMessage } from "../../src/shared/ExtensionMessage"

import { combineApiRequests as optimizedCombineApiRequests } from "../../src/shared/combineApiRequests"

function originalCombineApiRequests(messages: ClineMessage[]): ClineMessage[] {
	const combinedApiRequests: ClineMessage[] = []

	for (let i = 0; i < messages.length; i++) {
		if (messages[i].type === "say" && messages[i].say === "api_req_started") {
			let startedRequest = JSON.parse(messages[i].text || "{}")
			let j = i + 1

			while (j < messages.length) {
				if (messages[j].type === "say" && messages[j].say === "api_req_finished") {
					let finishedRequest = JSON.parse(messages[j].text || "{}")
					let combinedRequest = { ...startedRequest, ...finishedRequest }

					combinedApiRequests.push({
						...messages[i],
						text: JSON.stringify(combinedRequest),
					})

					i = j
					break
				}
				j++
			}

			if (j === messages.length) {
				combinedApiRequests.push(messages[i])
			}
		}
	}

	return messages
		.filter((msg) => !(msg.type === "say" && msg.say === "api_req_finished"))
		.map((msg) => {
			if (msg.type === "say" && msg.say === "api_req_started") {
				const combinedRequest = combinedApiRequests.find((req) => req.ts === msg.ts)
				return combinedRequest || msg
			}
			return msg
		})
}

function generateTestMessages(size: number, apiRatio: number = 0.4): ClineMessage[] {
    const messages: ClineMessage[] = [];
    let timestamp = 1000;

    const createStartMessage = (request: string): ClineMessage => ({
        type: "say",
        say: "api_req_started",
        text: `{"request":"${request}"}`,
        ts: timestamp++
    });

    const createFinishMessage = (cost: number): ClineMessage => ({
        type: "say",
        say: "api_req_finished",
        text: `{"cost":${cost}}`,
        ts: timestamp++
    });

    const createRegularMessage = (content: string): ClineMessage => ({
        type: "say",
        say: "text",
        text: content,
        ts: timestamp++
    });

    // Fill the array with the specified ratio of API requests vs regular messages
    for (let i = 0; i < size; i++) {
        if (Math.random() < apiRatio) {
            // Add a pair of API request messages
            messages.push(createStartMessage(`GET /api/data/${i}`));
            messages.push(createFinishMessage(0.001 * i));
        } else {
            // Add a regular message
            messages.push(createRegularMessage(`Message ${i}`));
        }
    }

    return messages;
}

function runBenchmark(messageCount: number, iterations: number = 100) {
    console.log(`\nBenchmark with ${messageCount} messages (${iterations} iterations):`);
    
    // Generate test data
    const testMessages = generateTestMessages(messageCount);
    
    let originalTotalTime = 0;
    let optimizedTotalTime = 0;
    
    // Run multiple iterations for more accurate results
    for (let i = 0; i < iterations; i++) {
        // Benchmark original implementation
        const originalStart = process.hrtime.bigint();
        originalCombineApiRequests(testMessages);
        const originalEnd = process.hrtime.bigint();
        const originalTime = Number(originalEnd - originalStart) / 1_000_000; // Convert to ms
        originalTotalTime += originalTime;
        
        // Benchmark optimized implementation
        const optimizedStart = process.hrtime.bigint();
        optimizedCombineApiRequests(testMessages);
        const optimizedEnd = process.hrtime.bigint();
        const optimizedTime = Number(optimizedEnd - optimizedStart) / 1_000_000; // Convert to ms
        optimizedTotalTime += optimizedTime;
    }
    
    // Calculate average times
    const originalAvg = originalTotalTime / iterations;
    const optimizedAvg = optimizedTotalTime / iterations;
    const improvement = (1 - (optimizedAvg / originalAvg)) * 100;
    
    console.log(`Original implementation: ${originalAvg.toFixed(3)} ms`);
    console.log(`Optimized implementation: ${optimizedAvg.toFixed(3)} ms`);
    console.log(`Performance improvement: ${improvement.toFixed(2)}%`);
}

function main() {
    console.log("Running combineApiRequests performance benchmarks");
    console.log("===============================================");

    runBenchmark(10, 10_000);
    runBenchmark(100, 10_00);
    runBenchmark(1_000, 100);
    runBenchmark(10_000, 100);
    runBenchmark(100_000, 10);
}

main();

Results:

Running combineApiRequests performance benchmarks
===============================================

Benchmark with 10 messages (10000 iterations):
Original implementation: 0.005 ms
Optimized implementation: 0.005 ms
Performance improvement: 1.78%

Benchmark with 100 messages (1000 iterations):
Original implementation: 0.032 ms
Optimized implementation: 0.034 ms
Performance improvement: -5.07%

Benchmark with 1000 messages (100 iterations):
Original implementation: 0.396 ms
Optimized implementation: 0.340 ms
Performance improvement: 14.20%

Benchmark with 10000 messages (100 iterations):
Original implementation: 17.095 ms
Optimized implementation: 3.885 ms
Performance improvement: 77.28%

Benchmark with 100000 messages (10 iterations):
Original implementation: 781.803 ms
Optimized implementation: 44.199 ms
Performance improvement: 94.35%

Copy link
Collaborator

@cte cte left a comment

Choose a reason for hiding this comment

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

🔥

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 5, 2025
@cte cte merged commit 8fe70e3 into RooCodeInc:main Apr 5, 2025
14 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Apr 5, 2025
@kyle-apex
Copy link
Contributor Author

Thanks for cleaning that up @cte !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants