This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: some code paths in ssl and _socket still import idna unconditionally
Type: resource usage Stage:
Components: Interpreter Core, Library (Lib), SSL Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, slingamn
Priority: normal Keywords: patch

Created on 2022-02-14 15:55 by slingamn, last changed 2022-04-11 14:59 by admin.

Pull Requests
URL Status Linked Edit
PR 31328 open slingamn, 2022-02-14 16:06
Messages (7)
msg413229 - (view) Author: Shivaram Lingamneni (slingamn) * Date: 2022-02-14 15:55
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 `idna_converter`.

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).
msg413231 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2022-02-14 16:32
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)
msg413237 - (view) Author: Shivaram Lingamneni (slingamn) * Date: 2022-02-14 17:00
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 #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 `urllib.parse` imports stringprep or unicodedata:

    python3 -c "import sys, urllib.parse; assert 'stringprep' not in sys.modules"

    python3 -c "import sys, urllib.parse; assert 'unicodedata' not in sys.modules"

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:

https://github.com/slingamn/mureq
msg413242 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2022-02-14 18:07
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.
msg413245 - (view) Author: Shivaram Lingamneni (slingamn) * Date: 2022-02-14 18:24
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.
msg413252 - (view) Author: Shivaram Lingamneni (slingamn) * Date: 2022-02-14 19:07
(Looks like it was 15 milliseconds when measuring inside `python -i`; I'm not sure what the root cause of the difference is, but clearly the 5 millisecond measurement from regular `python` is more accurate.)
msg413364 - (view) Author: Shivaram Lingamneni (slingamn) * Date: 2022-02-16 22:58
I wanted to check in about the status of this patch. Here's the case for the patch, as I understand it:

1. It is not a novel optimization, it just consistently applies design decisions that were made previously (RFE #1472176 and bpo-22127).
2. The performance impact of the initial import of encodings.idna and its transitive dependencies is in fact macroscopic relative to the baseline costs of the interpreter: 5 milliseconds to import the modules and 500 KB in increased RSS, relative to baselines of approximately 50 milliseconds to set up and tear down an interpreter and 10 MB in RSS.

Here are the relevant benchmarks, first for time:


```python
import time
start = time.time()
'a'.encode('idna')
print(time.time() - start)
```

and for memory:

```python
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?
History
Date User Action Args
2022-04-11 14:59:56adminsetgithub: 90906
2022-02-16 22:58:31slingamnsetmessages: + msg413364
2022-02-14 19:07:49slingamnsetmessages: + msg413252
2022-02-14 18:24:22slingamnsetmessages: + msg413245
2022-02-14 18:07:47christian.heimessetmessages: + msg413242
2022-02-14 17:00:34slingamnsetmessages: + msg413237
2022-02-14 16:32:14christian.heimessetassignee: christian.heimes ->
messages: + msg413231
stage: patch review ->
2022-02-14 16:06:41slingamnsetkeywords: + patch
stage: patch review
pull_requests: + pull_request29484
2022-02-14 15:55:15slingamncreate