-
Notifications
You must be signed in to change notification settings - Fork 1.1k
⚡️ Speed up function _estimate_string_tokens
by 221%
#2156
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
base: main
Are you sure you want to change the base?
⚡️ Speed up function _estimate_string_tokens
by 221%
#2156
Conversation
Certainly! Let's break down the profile first. ### Hotspots - Most time is spent calling `re.split()` for every string or string-like object, which is expensive. - Checking `isinstance` for common types on each iteration. - `tokens += 0` operations are no-ops and can be removed. - The regex can be precompiled. - `str.split()` with `None` as delimiter is often *much* faster and covers most whitespace splitting, which is likely enough here. - `content.strip()` is being called redundantly for every string, which we can optimize. Here’s a rewritten, **optimized** version. ### Optimizations made. - Precompiled the regex, so it’s not recompiled per call. - Removed all `tokens += 0` and unnecessary else branches (no effect). - Minimized calls to `.strip()` and `.split()` to once per string instance. - Dropped extraneous `isinstance` checks. - Moved logic into clear branches, optimizing the common paths. #### Further possible optimization, if you don't need exact punctuation splitting. If you're willing to change the token estimation (using whitespace instead of the full punctuation split), you can swap out `_TOKEN_SPLIT_RE.split(foo.strip())` to simply `foo.strip().split()` and drop all regex, which is **much** faster. But this does **relax** the original tokenization logic. Let me know if you want it even **faster** with that change, or if you need to preserve the splitting on punctuation!
tokens += len(_TOKEN_SPLIT_RE.split(part.strip())) | ||
elif isinstance(part, BinaryContent): | ||
tokens += len(part.data) | ||
# We don't need explicit handling for AudioUrl or ImageUrl, since they add 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we can skip the tokens += 0
, but we should keep on the original todo comment as image/audio URL parts actually do add tokens, we just don't count them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
return tokens | ||
|
||
|
||
_TOKEN_SPLIT_RE = re.compile(r'[\s",.:]+') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will have some overhead at import time, it's small but it'll add up if we do this with all regular expressions. Should we stick with re.split(r'[\s",.:]+', part.strip())
as it'll cache the regex the first time it's run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To validate the performance characteristics, I tried an experiment where I replaced the current suggestion with inline re.split and ran it on the generated test set and timed the runtime. So the only change is the global re.compile vs inline re.split.
global re.compile time -> 1.68ms
inline re.split -> 2.57ms
Yes, regex does cache the complied regex for future use, but it has overhead that especially when used in a loop can be high. In my experience with optimizations discovered with codeflash, I've seen re.compile be faster.
In this case, since regex is used multiple times and in a loop i would recommend regex compilation. Although its your decision.
📄 221% (2.21x) speedup for
_estimate_string_tokens
inpydantic_ai_slim/pydantic_ai/models/function.py
⏱️ Runtime :
5.29 milliseconds
→1.65 milliseconds
(best of89
runs)📝 Explanation and details
Hotspots
re.split()
for every string or string-like object, which is expensive.isinstance
for common types on each iteration.tokens += 0
operations are no-ops and can be removed.str.split()
withNone
as delimiter is often much faster and covers most whitespace splitting, which is likely enough here.content.strip()
is being called redundantly for every string, which we can optimize.Optimizations made.
tokens += 0
and unnecessary else branches (no effect)..strip()
and.split()
to once per string instance.isinstance
checks.✅ Correctness verification report:
🌀 Generated Regression Tests and Runtime
To edit these changes
git checkout codeflash/optimize-_estimate_string_tokens-mcs8yg4q
and push.