Skip to content

perf(llm): Optimize pruneLines functions in countTokens #5310

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

Merged
merged 4 commits into from
Jun 1, 2025

Conversation

0x23d11
Copy link
Contributor

@0x23d11 0x23d11 commented Apr 23, 2025

Description

Closes #4947

This PR addresses issue #4947 by optimizing the performance of the pruneLinesFromTop and pruneLinesFromBottom functions in core/llm/countTokens.ts

Problem

The previous implementations used Array.prototype.shift() and Array.prototype.pop() within a while loop to remove lines from the beginning or end of a prompt until it fit within the token limit. These array modification methods have a time complexity of O(n) because they require shifting subsequent elements. When dealing with very long prompts (e.g., thousands of lines), repeatedly calling shift() or pop() becomes computationally expensive, leading to significant performance degradation.

Solution

The implemented solution refactors these functions to avoid costly array modifications within the loop:

  1. The prompt is split into lines.
  2. The token count for each line is calculated once upfront and stored in an array (lineTokens).
  3. The total initial token count is calculated by summing lineTokens and adding the count for necessary newline characters (\n).
  4. A while loop iterates as long as the totalTokens exceeds maxTokens.
  5. Inside the loop, instead of removing elements from the lines array, an index pointer (start or end) is adjusted.
  6. The pre-calculated token count for the line being (conceptually) removed, along with its corresponding newline token, is subtracted from totalTokens.
  7. After the loop, Array.prototype.slice() (an O(n) operation performed only once) is used with the final start or end index to extract the desired lines.
  8. The resulting lines are joined back into a string.

Benefits

This approach drastically reduces the computational complexity, especially for large prompts, as the expensive O(n) operations (shift/pop) inside the loop are replaced by cheap O(1) index increments/decrements. The token calculation per line and the final slice operation are performed only once.

Checklist

  • I've read the contributing guide
  • [] The relevant docs, if any, have been updated or created
  • [] The relevant tests, if any, have been updated or created

Screenshots

[ For visual changes, include screenshots. Screen recordings are particularly helpful, and appreciated! ]

Testing instructions

Objective:

The primary goal of this PR is to optimize the performance of the pruneLinesFromTop and pruneLinesFromBottom utility functions. These functions are responsible for truncating a given string (prompt) by removing lines from the top or bottom, respectively, to ensure the resulting string does not exceed a specified maxTokens limit when processed by an LLM. This testing aims to verify that the optimized functions:
Correctly prune lines to meet the maxTokens constraint.
Maintain the existing logical behavior (i.e., correctly choose which lines to remove and which to keep).
Handle various edge cases appropriately.
Instructions:

  1. Automated Unit Tests (Primary Validation):

The most direct way to test these changes is by running the unit tests specifically written for these functions. We've recently added comprehensive tests for various scenarios.
Prerequisites: Ensure your development environment is set up and you can run tests.
Execution:
Navigate to the root directory of the continue project in your terminal.
Run the following command to execute the test suite for countTokens.ts:

npm test -- core/llm/countTokens.test.ts

Expected Results:

  • All tests within the describe("pruneLinesFromTop", ...) block should pass.
  • All tests within the describe("pruneLinesFromBottom", ...) block should pass.

These tests cover:

  • Basic pruning when the prompt exceeds maxTokens.
  • Cases where the prompt is already within maxTokens (no pruning should occur).
  • Edge cases such as an empty input prompt.
  • Edge cases such as maxTokens being 0.
  • Scenarios with single long lines that exceed maxTokens.
  • Correct pruning of multi-line prompts to specific token counts (based on assumptions made in the test cases for token costs of lines and newlines).

@0x23d11 0x23d11 requested a review from a team as a code owner April 23, 2025 12:11
@0x23d11 0x23d11 requested review from sestinj and removed request for a team April 23, 2025 12:11
Copy link

netlify bot commented Apr 23, 2025

Deploy Preview for continuedev ready!

Name Link
🔨 Latest commit 5543927
🔍 Latest deploy log https://app.netlify.com/projects/continuedev/deploys/6835b604511a340008add33f
😎 Deploy Preview https://deploy-preview-5310--continuedev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

@sestinj sestinj left a comment

Choose a reason for hiding this comment

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

This is a fairly complex change that affects a lot of things, so if we are going to consider merging this I would ask that it come with very good testing. Are you able to add unit tests that cover edge cases and make sure that this hasn't regressed in any way?

@RomneyDa
Copy link
Collaborator

RomneyDa commented Apr 23, 2025

@0x23d11 before writing tests for this note that #5138 changes pruning logic quite a bit and will effect this.

EDIT: didn't look closely enough, looks like this is for pruning lines not messages

@sestinj sestinj closed this Apr 23, 2025
@continuedev continuedev deleted a comment from sestinj Apr 23, 2025
@RomneyDa RomneyDa reopened this Apr 23, 2025
@RomneyDa
Copy link
Collaborator

@0x23d11 There are some tests in countTokens.test.ts that you could just flesh out a bit with more examples and then unskip!

@0x23d11
Copy link
Contributor Author

0x23d11 commented Apr 24, 2025

@RomneyDa ok I've seen the tests, I'll write some more examples for the tests.

After that it should be good to go right? Considering all the new test additions are good enough

@sestinj
Copy link
Contributor

sestinj commented Apr 29, 2025

@0x23d11 I'd love to merge this PR, please let me know if you have the chance to write some tests, or if you'd like any help!

@sestinj
Copy link
Contributor

sestinj commented May 7, 2025

Just wanted to bump this. I'm happy to leave it open for a while, but tests are important here since it's a very core piece of logic, and relatively complex

@sestinj
Copy link
Contributor

sestinj commented May 14, 2025

Hey @0x23d11, let me know if you have chance to look at this PR again. I'm probably going to close it if it becomes stale for longer. At this point just waiting on tests

@0x23d11
Copy link
Contributor Author

0x23d11 commented May 14, 2025

Hi @sestinj sorry for the delay, I'll add the tests by this weekend

@sestinj
Copy link
Contributor

sestinj commented May 26, 2025

@0x23d11 just wanted to bump this again. Let me know if you need any help

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 27, 2025
Copy link

github-actions bot commented May 27, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@0x23d11 0x23d11 requested a review from sestinj May 27, 2025 12:55
@0x23d11
Copy link
Contributor Author

0x23d11 commented May 27, 2025

I have read the CLA Document and I hereby sign the CLA

@0x23d11
Copy link
Contributor Author

0x23d11 commented May 29, 2025

@sestinj please take a look, I've added the tests

Copy link
Contributor

@sestinj sestinj left a comment

Choose a reason for hiding this comment

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

thanks for adding the tests, this looks great!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 1, 2025
@github-project-automation github-project-automation bot moved this from Todo to In Progress in Issues and PRs Jun 1, 2025
@sestinj sestinj merged commit 899a7d7 into continuedev:main Jun 1, 2025
33 of 34 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Jun 1, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Status: Done
3 participants