Autoload IPAddr instead of requiring it eagerly - #1099
Open
tas50 wants to merge 1 commit into
Open
Conversation
openssl/ssl.rb requires ipaddr at the top of the file. The only use is
in OpenSSL::SSL.verify_certificate_identity, and only for certificates
carrying an iPAddress SAN:
return true if san.value == IPAddr.new(hostname).hton
rescue IPAddr::InvalidAddressError
Autoload matches the shape of the change net/http is making for the
same constant and keeps IPAddr resolvable, so anything downstream that
relied on require "openssl" defining it is unaffected.
Note this does not save the socket require on the next line, which is
unconditional and unrelated. The saving is ipaddr.rb itself, 857 lines.
Measured with socket already loaded, which is openssl's actual
situation, on Ruby 4.0.6 (arm64-darwin), 15 runs:
marginal cost of require "ipaddr" min 2.47 ms / median 2.95 ms, 1 file
End-to-end require "openssl" moves from a min of 26.42 ms to 24.88 ms,
but that figure carries a lot of variance from loading openssl.so, so
the marginal number above is the honest one.
Small in absolute terms, but openssl is loaded in a large share of Ruby
processes, and the change is one line with no behavior difference.
The rescue clause still resolves correctly: IPAddr.new triggers the
autoload before it can raise, so IPAddr::InvalidAddressError is defined
by the time the rescue is evaluated.
Test suite: 630 tests, 0 failures, unchanged. The existing coverage
already exercises the IPAddr path directly, asserting
verify_certificate_identity against a cert with an IP:127.0.0.1 SAN.
The three added tests cover that requiring openssl does not load
ipaddr, that IPAddr still resolves afterwards, and that referencing it
pulls the library in. The first fails against the previous code.
Signed-off-by: Tim Smith <tsmith84@proton.me>
Member
|
I think it's generally better to avoid registering an autoload for another library. ipaddr is a small stdlib consisting of a single source file. Is lazy loading it actually a meaningful improvement? Also, wouldn't most users reach |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
lib/openssl/ssl.rbrequiresipaddrat the top of the file. The only use is inOpenSSL::SSL.verify_certificate_identity, and only for certificates carrying an iPAddress SAN:Fix
autoloadrather than an inline require, soIPAddrstays resolvable — anything downstream that relied onrequire "openssl"defining it is unaffected, and there is no per-verification cost.The rescue clause still resolves correctly:
IPAddr.newtriggers the autoload before it can raise, soIPAddr::InvalidAddressErroris defined by the time the rescue is evaluated.Measurements — and what this does not save
To be clear about scope: this does not save the
require "socket"on the very next line, which is unconditional and unrelated. The saving isipaddr.rbitself (857 lines).Measured with
socketalready loaded, which is openssl's actual situation, Ruby 4.0.6 (arm64-darwin), 15 runs:require "ipaddr"End-to-end
require "openssl"moves from a min of 40.68 → 35.12 ms in one sample and 26.42 → 24.88 ms in another; that figure carries a lot of variance from loadingopenssl.so, so the marginal number above is the honest one.Small in absolute terms, but
opensslis loaded in a large share of Ruby processes, the change is one line, and there is no behavior difference.Tests
bundle exec rake test: 630 tests, 0 failures, unchanged (633 with the additions).The existing suite already exercises the
IPAddrpath directly —test_ssl.rbassertsverify_certificate_identityagainst a certificate with anIP:127.0.0.1SAN, in both the matching and non-matching directions.The three added tests cover that requiring openssl does not load ipaddr, that
IPAddrstill resolves afterwards, and that referencing it pulls the library in. The first fails against the previous code.Related
ruby/net-http#337 makes the same change for
Resolvinnet/http, which uses it for two regexps on one line.