Skip to content

Use String#byteindex instead of String#index#286

Draft
shugo wants to merge 2 commits into
masterfrom
use_byteindex
Draft

Use String#byteindex instead of String#index#286
shugo wants to merge 2 commits into
masterfrom
use_byteindex

Conversation

@shugo

@shugo shugo commented May 23, 2024

Copy link
Copy Markdown
Member

Because String#index takes O(N), where N is the second argument, while String#byteindex takes O(1).

Because String#index takes O(N), where N is the second argument, while String#byteindex takes O(1).
@shugo

shugo commented May 23, 2024

Copy link
Copy Markdown
Member Author

We should wait until Ruby 3.1 gets EOL.

@nevans nevans added this to the v0.6 milestone May 23, 2024
@nevans

nevans commented May 23, 2024

Copy link
Copy Markdown
Collaborator

@shugo I added it to a new v0.6 milestone. 🙂

@admsev

admsev commented May 26, 2024

Copy link
Copy Markdown

@gooroodev WDYT?

@gooroodev

This comment was marked as spam.

@nevans

nevans commented May 27, 2024

Copy link
Copy Markdown
Collaborator

@admsev @gooroodev please don't. It's obnoxious and not very helpful.

@shugo

shugo commented Aug 13, 2024

Copy link
Copy Markdown
Member Author

I remember now that the argument of ResponseParser#parse is usually ASCII-8BIT, so this change doesn't matter unless ResponseParser#parse is directly used with other encodings.

@nevans nevans added the performance related to CPU use, memory use, latency, etc label Nov 10, 2025
@nevans nevans modified the milestones: v0.6, v1.0 Dec 10, 2025
@nevans

nevans commented Jan 9, 2026

Copy link
Copy Markdown
Collaborator

@shugo, as you said, our inputs are always ASCII-8BIT, so this isn't really needed. I do want it for semantic correctness. but I suspect #byteoffset vs #end will actually hurt performance due to the Array allocation. So this PR will need to wait until the min required ruby is 3.4 (after 2027-04-01) before we can use #byteend. (Alternatively, we could rewrite it with if MatchData.instance_methods.include?(:byteend), but I think that's maybe more hassle than is justified.)

@nevans nevans marked this pull request as draft April 4, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance related to CPU use, memory use, latency, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants