toke.c: Add conditional to skip memcmp() #23667
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tonycoz
reviewed
Sep 2, 2025
Member
|
Is this worth complicating the code? Is this particular part of the lexer a perfomance bottleneck? Hell, is the lexer in its entirety a bottleneck? |
Contributor
Author
|
The lexer is not a bottleneck, in its entirety. So, its a judgment call whether this complication is worth it. The original p.r. this idea is taken from #23533, and which was approved at one point, added tests at every place. I thought that made the code too hard to read, and am offering this alternative, which moves the complexity into a macro. I don't think this p.r. will have any noticeable impact on performance, but I don't think it makes things harder to comprehend. It might keep future code readers from making the same change. |
S_scan_str() calls memEQ() a bunch of times. The general case is that the strings are multiple bytes long, but most of the time, at least one of the operands will be just a single byte. We can avoid a libc call by just comparing the single bytes when the length is 1. Tony Cook is of the opinion that other uses of memEQ() will more likely be multiple bytes, so adding this check to all calls would not be beneficial. I looked through the core source, and agree. So this adds a macro that tests for a single byte, and if multiple calls plain memEQ(). Spotted by Daniel Dragan Since the tokenizer is not hot code, this won't make a noticeable difference in performance. I think the reason to do it is mainly to show it has been done for people in the future who would otherwise come along and notice this
khwilliamson
force-pushed
the
memEQ1
branch
from
b16c505 to
6eb0d35
Compare
khwilliamson
added a commit
that referenced
this pull request
Sep 22, 2025
Somehow this got left out of the rebase for GH #23667
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
S_scan_str() calls memEQ() a bunch of times. The general case is that the strings are multiple bytes long, but most of the time, at least one of the operands will be just a single byte. We can avoid a libc call by just comparing the single bytes when the length is 1.
Tony Cook is of the opinion that other uses of memEQ() will more likely be multiple bytes, so adding this check to all calls would not be beneficial. I looked through the core source, and agree. So this adds a macro that tests for a single byte, and if multiple calls plain memEQ().
Spotted by Daniel Dragan