Skip to content
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

simplify matchplus and matchstar #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marler8997
Copy link

matchplus can be simplified by only modifying matchlength once the complete match is successful. This means it doesn't have to rewind matchlength as it iterates through each possible matchpattern. This also means it keeps matchlength unmodified if it doesn't return a match. Because of this last part, this also means that matchstar can leverage matchplus which reduces it to single line of code return matchplus(...) || matchpattern(..).

`matchplus` can be simplified by only modifying `matchlength` once the complete match is successful.  This means it doesn't have to rewind `matchlength` as it iterates through each possible `matchpattern`.  This also means it keeps `matchlength` unmodified if it doesn't return a match.  Because of this last part, this also means that `matchstar` can leverage `matchplus` which reduces it to single line of code `return matchplus(...) || matchpattern(..)`.
@marler8997
Copy link
Author

Very interestingly, this change has noticeably improved performance. Before this change, running time ./tests/test2 3 times in a row gave these times:

real	0m4.047s
real	0m5.525s
real	0m3.952s

After this change I'm consistently getting below 3.6:

real	0m3.577s
real	0m3.571s
real	0m3.587s

The run times are much more consistent and it's around a 25% improvement for that test. My guess is that this performance may be due to less code needed to be swapped in and out of the icache? Not sure, but interesting.

matyalatte added a commit to matyalatte/tiny-str-match that referenced this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants