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

feat: optimisation of EqualMatch method #3342

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sanix-Darker
Copy link

@Sanix-Darker Sanix-Darker commented Jun 23, 2023

PROPOSAL

Testing some optimisations i wanted to check :

  • Combined whitespace stripping: The leading and trailing whitespaces are stripped in a single step, reducing the number of method calls and condition checks.

  • Reduced string conversions: Instead of converting the entire text and pattern to strings, only the necessary substrings are used. This avoids unnecessary conversions and improves performance.

  • Early return: The function returns early if the normalized characters or string comparison fails, eliminating the need for the match boolean variable and reducing unnecessary computations.

    - Combined whitespace stripping: The leading and trailing whitespaces are stripped in a single step, reducing the number of method calls and condition checks.

    - Reduced string conversions: Instead of converting the entire text and pattern to strings, only the necessary substrings are used. This avoids unnecessary conversions and improves performance.

    - Early return: The function returns early if the normalized characters or string comparison fails, eliminating the need for the match boolean variable and reducing unnecessary computations.
@Sanix-Darker Sanix-Darker marked this pull request as draft June 23, 2023 21:48
@Sanix-Darker Sanix-Darker marked this pull request as ready for review June 23, 2023 21:58
@Sanix-Darker
Copy link
Author

Hello, I didn't create an issue for this PR, sorry for that, do you want me to provide more details with benchmarks ? @junegunn

@junegunn
Copy link
Owner

I didn't create an issue for this PR

No problem with that.

more details with benchmarks

That would be nice, yes. But if the suggested change is succinct and it makes the code objectively better, then we can merge it even if it doesn't bring a significant performance boost.


Just out of curiosity, can you tell me why you started looking into improving EqualMatch? which is 1. rarely used (only triggered when you type in ^PAT$), 2. too simple to have performance issues in real-world use cases, at least from my experience.

@Sanix-Darker
Copy link
Author

Sanix-Darker commented Jun 29, 2023

just out of curiosity, can you tell me why you started looking into improving EqualMatch?

I am a GIGA big fan and a daily user of fzf, so I was wondering how it works, I started reading the code and I wanted to provide my optimization "point of view" on some methods for this module, but in multiple small and different PRs, that's why I started with the smallest and easiest one EqualMatch here!

  1. too simple to have performance issues in real-world use cases, at least from my experience.

You're right for that, I will be glad to work on other "most important perfs issues" from your experience if you have some to provide me !

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