bpo-31659: ssl: Don't use textwrap.fill() to format PEM cert#3849
Conversation
DER_cert_to_PEM_cert() used textwrap.fill() to format PEM. But it's library to wrap lines on word boundary. And importing textwrap is little slow.
methane
changed the title
tiran
left a comment
tiran
left a comment
There was a problem hiding this comment.
I like the general idea but not the execution.
| textwrap.fill(f, 64) + '\n' + | ||
| PEM_FOOTER + '\n') | ||
| ss = [PEM_HEADER] | ||
| ss += [f[i:i+64] for i in range(0, len(f), 64)] |
There was a problem hiding this comment.
+1 for the idea
Please don't use += and use a self-explaining local variable name:
lines = [PEM_HEADER]
lines.extend(f[i:i+64] for i in range(0, len(f), 64))
lines.extend([PEM_FOOTER, ''])
return '\n'.join(lines)
There was a problem hiding this comment.
My first impulse was to add a similar comment. But actually this doesn't matter. All lists here are small I hope, so copying lists doesn't add much overhead. On other hand, the variant with extend() and generator expression add their own overheads.
- Generator expression is slightly slower than a list comprehension.
list.extend()is special cased for tuples and list. It is slower with general iterators.- Calling a method is slower than executing an operator.
- I didn't checked, but maybe
PEM_FOOTER + '\n'is faster than[PEM_FOOTER, ''].
Hence the current code looks good enough to me. It is enough fast and looks enough Pythonic. If there are opportunities to enhance it, it is not worth the spent time.
There was a problem hiding this comment.
I'm sorry about variable name. I didn't care about name for very short scope variables.
In case of performance, as serhiy said, cert size won't be arbitrary large.
And old code used temporal string for body part. This commit didn't make anythong worse.
$ ./python -m perf timeit -s "import textwrap; s='a'*1000" -- 'textwrap.fill(s, 60)'
.....................
Mean +- std dev: 192 us +- 10 us
$ ./python -m perf timeit -s "import textwrap; s='a'*1000" -- 'x=[]; x+=[s[i:i+60] for i in range(0,len(s),60)]; "\n".join(x)'
.....................
Mean +- std dev: 4.66 us +- 0.06 us
6 participants
DER_cert_to_PEM_cert() used textwrap.fill() to format PEM.
But it's library to wrap lines on word boundary, while PEM is
base64 encoded string.
Additionally, importing textwrap is little slow.
https://bugs.python.org/issue31659