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.

Sign up for GitHub

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

Jump to bottom

Use single-char variant of String.indexOf() where possible #105205

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Use single-char variant of String.indexOf() where possible #105205

merged 3 commits into from
Feb 12, 2024

Conversation

sabi0
Copy link
Contributor

@sabi0 sabi0 commented Feb 6, 2024

indexOf(char) is more efficient than searching for the same one-character String.

@sabi0
Use single-char variant of String.indexOf() where possible
167b31e 
indexOf(char) is more efficient than searching for the same one-character String.
@sabi0 sabi0 requested a review from a team as a code owner February 6, 2024 16:46
@elasticsearchmachine elasticsearchmachine added v8.13.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Feb 6, 2024
@kingherc kingherc self-assigned this Feb 7, 2024
@kingherc kingherc added the :Core/Infra/Core Core issues without another label label Feb 7, 2024
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team and removed needs:triage Requires assignment of a team area label labels Feb 7, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@kingherc
Copy link
Contributor

kingherc commented Feb 7, 2024

buildkite test this

@kingherc kingherc removed their assignment Feb 7, 2024
@kingherc
Copy link
Contributor

kingherc commented Feb 7, 2024

Thanks for the PR ! Could you please fix formatting? (see https://github.com/elastic/elasticsearch/blob/main/CONTRIBUTING.md#formatting )

I will let the Core team review this PR to also comment on the performance of using a char vs a string.

@kingherc
Copy link
Contributor

kingherc commented Feb 7, 2024

buildkite test this

@williamrandolph
Copy link
Contributor

Looking at the code in Java's String, StringLatin1, and StringUTF16, the single-character search does go into a simpler algorithm. I don't know how this would hold up in a benchmark, but on the face of things I think the change makes sense.

@williamrandolph williamrandolph self-assigned this Feb 9, 2024
@williamrandolph
Copy link
Contributor

@elasticsearchmachine update branch

@williamrandolph
Copy link
Contributor

buildkite test this

williamrandolph
williamrandolph approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@williamrandolph williamrandolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good and tests pass. I'm going to merge.

@williamrandolph williamrandolph merged commit a50e58d into elastic:main Feb 12, 2024
16 checks passed
@sabi0 sabi0 deleted the string_indexof branch February 12, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamrandolph williamrandolph williamrandolph approved these changes

Assignees

@williamrandolph williamrandolph

Labels
:Core/Infra/Core Core issues without another label external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue Team:Core/Infra Meta label for core/infra team v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sabi0 @elasticsearchmachine @kingherc @williamrandolph @elasticmachine