Skip to content

perf: replace startsWith with charAt #448

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

Closed
wants to merge 1 commit into from

Conversation

btea
Copy link

@btea btea commented Jun 22, 2024

What is the purpose of this pull request?

refer

// We do not use `startsWith` because this is 10x slower than current implementation for some cases.
// eslint-disable-next-line @typescript-eslint/prefer-string-starts-ends-with

What changes did you make? (Give an overview)

@mrmlnc
Copy link
Owner

mrmlnc commented Jun 23, 2024

Thanks for the pull request.

I think I would prefer to keep the linter rule and continue using the .startsWith method instead of the .charAt method in most cases.

The difference between the place you indicated in the pull request and the others is that it is a very hot. The removeLeadingDotSegment method is called for each found entity. For real directories, it can be tens of thousands of calls. The performance of such methods needs to be improved.

In other methods (like the ones you changed in this pull request), the frequency of calls is much, much lower. For patterns, we can talk about dozens of calls, no more. At worst, it could be hundreds of calls if the user specifies hundreds of patterns. But I think he will run into another performance problem like pattern matching.

Thank you for your pull request. If you are aware of any cases where your changes have led to a noticeable improvement in speed, please let me know.

@mrmlnc mrmlnc closed this Jun 23, 2024
@btea btea deleted the perf/use-charAt-replace-startsWith branch June 23, 2024 11:59
@btea
Copy link
Author

btea commented Jun 23, 2024

Thanks for the detailed instructions.

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