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
some code paths in ssl and _socket still import idna unconditionally #90906
Comments
Importing the idna encoding has a significant time and memory cost. Therefore, the standard library tries to avoid importing it when it's not needed (i.e. when the domain name is already pure ASCII), e.g. in Lib/http/client.py and Modules/socketmodule.c with However, there are code paths that still attempt to encode or decode as idna unconditionally, in particular Lib/ssl.py and _socket.getaddrinfo. Here's a one-line test case: python3 -c "import sys, urllib.request; urllib.request.urlopen('https://www.google.com'); assert 'encodings.idna' not in sys.modules" These code paths can be converted using existing code to do the import conditionally (I'll send a PR). |
Please provide benchmarks and data for your claim that encodings.idna is a performance bottleneck. encodings.idna is a simple, short module without state. On my system it takes about 0.15 msec to import the module. When unicodedata and stringprep aren't loaded yet, it still takes less than 0.5 msec. The stringprep and unicodedata modules are used by other modules, e.g. urllib parse. It's likely that any non-trivial program with network access has both imported already. $ python3 -m timeit -s "import sys" "import encodings.idna; sys.modules.pop('encodings.idna'); sys.modules.pop('stringprep'); sys.modules.pop('unicodedata')"
500 loops, best of 5: 488 usec per loop The IDNA codec performs additional verification of the input. You cannot replace it with a simple "try encode to ASCII" block: >>> ("a"*65).encode('idna')
Traceback (most recent call last):
File "/usr/lib64/python3.10/encodings/idna.py", line 167, in encode
raise UnicodeError("label too long")
UnicodeError: label too long
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
UnicodeError: encoding with 'idna' codec failed (UnicodeError: label too long) |
Thanks for the prompt response. As evidence that this was of concern to the development team in the past, here's an issue where the unnecessary import of idna was treated as a regression: https://bugs.python.org/issue22127 The discussion there also examines the semantic change produced by the optimization (some invalid labels making it to a DNS lookup instead of being rejected) and doesn't consider it to be a breaking change (albeit a reason not to backport). (I also see references in documentation to a discussion labeled "RFE bpo-1472176", but am unable to find the actual bug tracker or database entry this refers to.) A time cost of 15 milliseconds seems accurate to me. The RAM cost on my release build of Python 3.8.10 is about 600 KB in RSS (this is approximately 5% of the baseline interpreter usage). I cannot reproduce the claim that
I am developing a new lightweight http library that does use urllib.parse; on my system, these patches allow it to function without importing stringprep, idna, or unicodedata: |
It's 100 times faster. The initial import takes about 150 μsec (0.15msec) and it's a one time cost. IDNA encoding of a typical hostname is about 70nsec. A DNS lookup is three magnitudes slower. |
Sorry, I should have been more clear: I am including the initial costs of importing stringprep and unicodedata. On my system: $ python3 -c "import time; start = time.time(); r = 'a'.encode('idna'); elapsed = time.time() - start; print(elapsed)"
0.0053806304931640625 So the earlier measurement of 15 milliseconds was excessive (I'm not sure what happened) but it's the right order of magnitude: I can reproduce 5 milliseconds reliably. |
(Looks like it was 15 milliseconds when measuring inside |
I wanted to check in about the status of this patch. Here's the case for the patch, as I understand it:
Here are the relevant benchmarks, first for time: import time
start = time.time()
'a'.encode('idna')
print(time.time() - start) and for memory: import os
def rss():
os.system('grep VmRSS /proc/' + str(os.getpid()) + '/status')
rss()
'a'.encode('idna')
rss() Are there potential changes to this patch that would mitigate your concerns? |
slingamn mannequin commentedFeb 14, 2022
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: