Skip to content

Conversation

@lzap
Copy link
Contributor

@lzap lzap commented Oct 7, 2020

Hello,

URI#hostname extends URI#host with IPv6 support. In URI, IPv6 address must have square brackets (e.g. http://[2001:db8::1]), URI#hostname strips these characters out while URI#hostname= adds them if missing. There are three regular expressions to perform these tasks which can dramatically slow down performance. I am attaching a two-line patch and here is a benchmark: https://gist.github.com/lzap/24cbecb47daf29111350e41a24250922

In short, URI#hostname is 86-89% and hostname= is 55-154% faster according to my measurements.

For the record, I created a Ruby MRI RM ticket https://bugs.ruby-lang.org/issues/17219 as I had no idea URI is a separate gem hosted on github.

#
def hostname=(v)
v = "[#{v}]" if /\A\[.*\]\z/ !~ v && /:/ =~ v
v = "[#{v}]" if v&.index(':') && !v.start_with?('[') && !v.end_with?(']')
Copy link
Member

Choose a reason for hiding this comment

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

/\A\[.*\]\z/ !~ v and !v.start_with?('[') && !v.end_with?(']') differ.
It should be !(v.start_with?('[') && v.end_with?(']')).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What an overlook, shame on me, this is 101. I will amend the fix. Also, I think I should start with start_with in the statement because index call is more costly and it is more likely to not match in the future as the world progresses towards IPv6.

@lzap lzap force-pushed the faster-hostname-17219 branch from b3a035f to 36c3fd7 Compare October 7, 2020 13:59
@lzap
Copy link
Contributor Author

lzap commented Oct 7, 2020

Amended the change with force-push.

@krzysiek1507
Copy link

Could you please check if there are tests for those methods?

@lzap
Copy link
Contributor Author

lzap commented Oct 7, 2020

Could you please check if there are tests for those methods?

Yes, I can actually amend one more change - an extra edge case and a case when square brackets are already present. Lemme do that.

@lzap lzap force-pushed the faster-hostname-17219 branch from 36c3fd7 to 599d2b9 Compare October 7, 2020 14:11
@krzysiek1507
Copy link

@lzap It wasn't me who meant start_with? and end_with?. It was nobu ruby/ruby#3635 (comment)
Could you please fix it here? https://bugs.ruby-lang.org/issues/17219#note-2

@lzap
Copy link
Contributor Author

lzap commented Oct 12, 2020

Oh I am sorry, fixed!

@jeremyevans
Copy link
Contributor

Sorry for the delay, this looks good, will merge shortly.

@jeremyevans jeremyevans merged commit 3b7ccfd into ruby:master Mar 4, 2021
@lzap lzap deleted the faster-hostname-17219 branch April 14, 2021 10:01
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.

4 participants