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

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

Copy link
Copy Markdown
MemberAuthor

We should wait until Ruby 3.1 gets EOL.

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

Copy link
Copy Markdown
Collaborator

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

@admsev

Copy link
Copy Markdown

@gooroodev WDYT?

@gooroodev

This comment was marked as spam.

@nevans

Copy link
Copy Markdown
Collaborator

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

@shugo

Copy link
Copy Markdown
MemberAuthor

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.

@nevansnevans added the performance related to CPU use, memory use, latency, etc label Nov 10, 2025
@nevansnevans modified the milestones: v0.6, v1.0Dec 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performancerelated to CPU use, memory use, latency, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@shugo@nevans@admsev@gooroodev