-
Notifications
You must be signed in to change notification settings - Fork 22k
Optimise ActionDispatch::Http::URL.build_host_url
when protocol in host
#55641
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
Optimise ActionDispatch::Http::URL.build_host_url
when protocol in host
#55641
Conversation
Could the three branches be combined with something like this? diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb
index 7570a6a9af..12632736e8 100644
--- a/actionpack/lib/action_dispatch/http/url.rb
+++ b/actionpack/lib/action_dispatch/http/url.rb
@@ -202,15 +202,15 @@ def extract_subdomains_from(host, tld_length)
def build_host_url(host, port, protocol, options, path)
if match = host.match(HOST_REGEXP)
- protocol ||= match[1] unless protocol == false
+ protocol_from_host = match[1] unless protocol == false
host = match[2]
port = match[3] unless options.key? :port
end
- protocol = normalize_protocol protocol
+ protocol = protocol_from_host || normalize_protocol(protocol).dup
host = normalize_host(host, options)
- result = protocol.dup
+ result = protocol
if options[:user] && options[:password]
result << "#{Rack::Utils.escape(options[:user])}:#{Rack::Utils.escape(options[:password])}@" |
This line if protocol.nil?
protocol = protocol_from_host || normalize_protocol(nil)
else
protocol = normalize_protocol(protocol).dup
end but I think the current approach is more idiomatic. |
Ah, right. I think the diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb
index 7570a6a9af..12632736e8 100644
--- a/actionpack/lib/action_dispatch/http/url.rb
+++ b/actionpack/lib/action_dispatch/http/url.rb
@@ -202,15 +202,15 @@ def extract_subdomains_from(host, tld_length)
def build_host_url(host, port, protocol, options, path)
if match = host.match(HOST_REGEXP)
- protocol ||= match[1] unless protocol == false
+ protocol_from_host = match[1] if protocol.nil?
host = match[2]
port = match[3] unless options.key? :port
end
- protocol = normalize_protocol protocol
+ protocol = protocol_from_host || normalize_protocol(protocol).dup
host = normalize_host(host, options)
- result = protocol.dup
+ result = protocol
if options[:user] && options[:password]
result << "#{Rack::Utils.escape(options[:user])}:#{Rack::Utils.escape(options[:password])}@" |
a22f19d
to
45c7cf6
Compare
I like it! I've gone ahead and actioned this, however, I still need the result dup branch because the I've also gone ahead and updated the benchmark results with the latest change just in case. TL;DR, the improvement is still around the same. |
45c7cf6
to
1fcea88
Compare
end | ||
|
||
protocol = normalize_protocol protocol | ||
protocol = protocol_from_host || normalize_protocol(protocol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously this would always normalize protocol
.
Does this skip normalizing protocol_from_host
now when the regexp matches?
Edit Nevermind, that's the purpose of this whole PR 🙃
1fcea88
to
f4794fd
Compare
f4794fd
to
9a5215e
Compare
I added a test on main that fails with the current code (addc45c), and added a commit to fix it. Let me know if it looks good and I'll squash + merge 👍 |
LGTM, thank you! Please feel free to add yourself to the changelog entry when squashing! |
9a5215e
to
7986bc3
Compare
…host Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
7986bc3
to
36785ad
Compare
Motivation / Background
When using URL helpers with a host that includes the protocol (e.g.,
{ host: "https://api.example.com" }
), we (@buildkite) found that the already-extracted protocol was being unnecessarily processed through#normalize_protocol
, creating extra allocations and showing up as a hotspot in our production profiles.Example local profile: output.json.gz (viewable at https://vernier.prof, select allocations for data source)
Detail
Before:
HOST_REGEXP
extracts"https://"
from hostnormalize_protocol
processes"https://"
throughPROTOCOL_REGEXP
and creates new stringprotocol.dup
creates another allocationAfter:
HOST_REGEXP
extracts"https://"
from hostnormalize_protocol
since it's already in correct formatdup
since regex capture groups are mutableThis optimization only applies when no explicit protocol option is provided and
HOST_REGEXP
successfully extracts a protocol. TheHOST_REGEXP
pattern ensures extracted protocols are always in the final"protocol://"
format thatnormalize_protocol
would return, making the normalization step redundant.The change eliminates 2 string allocations per URL generation when protocol is included in the host, resulting in a ~10-12% performance improvement and ~16% reduction in total allocations for that case. Other cases remain unaffected with no performance regression.
Benchmarking
Script:
Results:
Additional information
N/A
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]