-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-101178: Add Ascii85, Base85, and Z85 support to binascii #102753
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
base: main
Are you sure you want to change the base?
Conversation
|
It's a year later, and Z85 support has been added to For reference, this is the benchmark run that led me to do so. # After merging main but before adding Z85 support to this PR
(cpython-b85) $ python bench_b85.py 64
b64encode : 67108864 b in 0.121 s (527.435 MB/s) using 42.667 MB
b64decode : 89478488 b in 0.309 s (276.188 MB/s) using 56.889 MB
a85encode : 67108864 b in 0.297 s (215.150 MB/s) using 0.000 MB
a85decode : 83886080 b in 0.205 s (390.751 MB/s) using 0.000 MB
b85encode : 67108864 b in 0.106 s (604.359 MB/s) using 0.000 MB
b85decode : 83886080 b in 0.204 s (393.040 MB/s) using 0.000 MB
z85encode : 67108864 b in 0.204 s (313.610 MB/s) using 80.000 MB
z85decode : 83886080 b in 0.300 s (266.670 MB/s) using 100.000 MBThe existing Z85 implementation translates from the standard base85 alphabet to Z85 after the fact and within Python, so it was already benefiting from this PR but with substantial performance and memory usage overhead. That overhead is now gone. |
71f1955 to
7b4aba1
Compare
Add Ascii85, base85, and Z85 encoders and decoders to `binascii`, replacing the existing pure Python implementations in `base64`. No API or documentation changes are necessary with respect to `base64.a85encode()`, `b85encode()`, etc., and all existing unit tests for those functions continue to pass without modification. Note that attempting to decode Ascii85 or base85 data of length 1 mod 5 (after accounting for Ascii85 quirks) now produces an error, as no encoder would emit such data. This should be the only significant externally visible difference compared to the old implementation. Resolves: pythongh-101178
7b4aba1 to
05ae5ad
Compare
|
PR has been rebased onto main at 78cfee6 with squashing. |
I believe you have to document this change. |
Fair point, I could do that. In case anyone argues for keeping the old behavior (silently ignoring length 1 mod 5), I won't do it just yet. |
If we were strictly following PEP-0399, _base64 would be a C module for accelerated functions in base64. Due to historical reasons, those should actually go in binascii instead. We still want to preserve the existing Python code in base64. Parting out facilities for accessing the C functions into a module named _base64 shouldn't risk a naming conflict and will simplify testing.
This is done differently to PEP-0399 to minimize the number of changed lines.
As we're now keeping the existing Python base 85 functions, the C implementations should behave exactly the same, down to exception type and wording. It is also no longer an error to try to decode data of length 1 mod 5.
|
The PR has been updated to preserve the existing base 85 Python functions in |
Importing update_wrapper() from functools to copy attributes is expensive. Do it ourselves for only the most relevant ones.
This requires some code duplication, but oh well.
Using a decorator complicates function signature introspection.
Do we really need to test the legacy API twice?
It may be better to separate them for PGO though I have no idea whether there will be an impact or not (this needs to be measured). If you're worried about parts of the code being duplicated, it can be refactored into macros (or into smaller functions). However, we should avoid the user-interface exposing different flavors with boolean switches (we usually try to avoid this, as illustrated by examples of |
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
I pushed these changes. The code is similar to See also #143216 -- it reuses the same code for wrapping line in
Yes, of course. |
|
Hi @picnixz, thanks for reviewing.
I did attempt to follow PEP-7; did you have something specific in mind? If it's about the old-style variable declarations at the top of each C function, the reason for that was to match much of the rest of But since @serhiy-storchaka partially undid that in 0df9a40 (apparently following ca99af3) I don't mind cleaning up the style a bit more. |
|
It's mainly to avoid Py_ssize_t out_len = 5 * ((bin_len + 3) / 4);
if (wrap) out_len += 4;
if (!pad && (bin_len % 4)) out_len -= 4 - (bin_len % 4);
if (width && out_len) out_len += (out_len - 1) / width;I prefer clear |
Will do. |
Per [1] this will be allowed despite PEP-0399. [1]: https://discuss.python.org/t/accelerator-for-ascii85-base85/105415/3
We are no longer testing both Python and C codepaths for base-85-related functions in base64.
This avoids exposing a `z85` bool parameter. Also update documentation. While we're at it, refer to the encoding as "Base85" instead of "base85".
|
The PR has been updated. To summarize the changes:
I also updated the PR description with another round of benchmarks. Notably, the optimizations to Regarding C code organization in Weirdly, Z85 decoding is about 10% slower for me with PGO/LTO enabled. That might mean the Z85 tests actually are causing the compiler to optimize differently, or it might be an artifact of my ancient dev machine or my crude benchmarking methods. In any case the performance gains compared to the pure-Python implementations are still quite substantial so I'm going to leave it at that. |
serhiy-storchaka
left a comment
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.
| fold_spaces: bool = False | ||
| Allow 'y' as a short form encoding four spaces. | ||
| wrap: bool = False | ||
| Expect data to be wrapped in '<~' and '~>' as in Adobe Ascii85. | ||
| ignore: Py_buffer(c_default="NULL", py_default="b''") = None | ||
| An optional bytes-like object with input characters to be ignored. |
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.
I have a question -- why do you use different parameter names than in base64.a85encode()?
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.
I just chose some names that sounded OK to me. I didn't like adobe in base64.a85decode() because it seemed too specific, and there wasn't commonality between binascii and base64 parameter names (lots of legacy code everywhere).
After reading #143262 and #143216 I gather that reusing CPython interned strings is implicitly good practice, so I will try to do that if you prefer.
Modules/binascii.c
Outdated
| ascii_len -= 2; | ||
| if (ascii_len >= 2 | ||
| && ascii_data[0] == BASE85_A85_PREFIX | ||
| && ascii_data[1] == BASE85_A85_AFFIX) { |
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.
PEP 7: if the condition takes multiple lines, move { at a separate line.
| && ascii_data[1] == BASE85_A85_AFFIX) { | |
| && ascii_data[1] == BASE85_A85_AFFIX) | |
| { |
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.
Thanks, missed this, will do.
Modules/binascii.c
Outdated
| Py_ssize_t bin_len = ascii_len; | ||
| unsigned char this_ch = 0; | ||
| for (Py_ssize_t i = 0; i < ascii_len; i++) { | ||
| this_ch = ascii_data[i]; | ||
| if (this_ch == 'y' || this_ch == 'z') { | ||
| bin_len += 4; | ||
| } | ||
| } | ||
| bin_len = 4 * ((bin_len + 4) / 5); |
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.
This calculation is not accurate, because zs and ys already accounted in the total number of characters. It is also possible to get an integer overflow on 32-bit platform -- try to decode a string of 2**29 zs.
Count separately the number of zs and ys, then the output size is (bin_len - count + 4) / 5 * 4 + count * 4, but check for integer overflow.
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.
Could you explain what you mean? z and y are respectively equivalent to !!!!! and +<VdL, so it's correct to add 4 to ascii_len for each one encountered.
To be thorough I wrote the following to investigate if there are differences between the current and proposed output length calculation. It prints diffs: 0.
#include <stdio.h>
int main() {
int diffs = 0;
for (int ascii_len = 1; ascii_len <= 1024; ascii_len++) {
for (int count_yz = 0; count_yz <= ascii_len; count_yz++) {
/* Current output length calculation */
int adj_ascii_len = ascii_len + 4 * count_yz;
int bin_len = 4 * ((adj_ascii_len + 4) / 5);
/* Proposed output length calculation */
int bin_len_2 = (ascii_len - count_yz + 4) / 5 * 4 + count_yz * 4;
if (bin_len != bin_len_2) {
printf("ascii_len %d count_yz %d bin_len %d bin_len_2 %d\n",
ascii_len, count_yz, bin_len, bin_len_2);
diffs++;
}
}
}
printf("diffs: %d\n", diffs);
return 0;
}I didn't check for integer overflow because the rest of binascii doesn't, but I can add it.
| Allow 'y' as a short form encoding four spaces. | ||
| wrap: bool = False | ||
| Expect data to be wrapped in '<~' and '~>' as in Adobe Ascii85. | ||
| ignore: Py_buffer(c_default="NULL", py_default="b''") = None |
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.
None is not acceptable value, isn't? Use b''.
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.
If the signature of a2b_ascii85(string, /, *, fold_spaces=False, wrap=False, ignore=b"") was ignore=None, this would be acceptable. Because it isn't, this clinic input should be ignore: Py_buffer(c_default="NULL", py_default="b''") = b'' instead. Is that what you meant?
Modules/binascii.c
Outdated
| Emit 'y' as a short form encoding four spaces. | ||
| wrap: bool = False | ||
| Wrap result in '<~' and '~>' as in Adobe Ascii85. | ||
| width: unsigned_int(bitwise=True) = 0 |
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.
bitwise=True make it to wrap, so 2**32 is interpreted as 0.
There is also no benefit of restricting the range to the C's int, it have to support sys.maxsize. So use size_t here. We can also use a special converter, because the current Python code does not have limitation, and negative values interpreted as 1 (although this is an implementation detail, not documented and not tested, so it is not necessary to support this).
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.
I'll investigate this further and either use size_t or a custom converter.
Modules/binascii.c
Outdated
| } | ||
|
|
||
| /* Allocate output buffer. | ||
| XXX: Do a pre-pass above some threshold estimate (cf. 'yz')? |
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.
I do not think it is worth it.
| data: ascii_buffer | ||
| / | ||
| * | ||
| strict_mode: bool = False |
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.
Do we need this? It is always True in base64. strict_mode=False is legacy.
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.
Maybe not so legacy, a strict_mode parameter was added to binascii.a2b_base64() in #24402 for what seem like good security-related reasons that are possibly relevant for binascii.a2b_base85() and binascii.a2b_z85() as well.
| * | ||
| pad: bool = False | ||
| Pad input to a multiple of 4 before encoding. | ||
| newline: bool = True |
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.
Do we need this? It is always False in base64. newline=True is legacy.
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.
For this one I agree we probably don't really need it. I put it in following the example of binascii.b2a_base64(). Note that binascii.b2a_uu() always appends a newline but binascii.b2a_hex() doesn't. If you prefer I can remove it.
No performance changes. Base85 tables were probably already aligned.
Include an integer overflow check for Ascii85.
Performance gains of up to 8% for a2b_ascii85() and 25% for a2b_base85() and a2b_z85() were observed.
|
The PR has been updated to address most reviewer comments. I even found some minor decoding performance gains. A bit more polish and this one will be done! |
Synopsis
Add Ascii85, Base85, and Z85 encoder and decoder functions implemented in C to
binasciiand use them to greatly improve the performance and reduce the memory usage of the existing Ascii85, Base85, and Z85 codec functions inbase64.No API or documentation changes are necessary with respect to any functions in
base64, and all existing unit tests for those functions continue to pass without modification.Resolves: gh-101178
Discussion
The base85-related functions in
base64are now wrappers for the new functions inbinascii, as envisioned in the docs:Parting out Ascii85 from Base85 and Z85 was warranted in my testing despite the code duplication due to the various performance-murdering special cases in Ascii85.
Comments and questions are welcome.
Benchmarks
Updated December 28, 2025.
The old pure-Python implementation is two orders of magnitude slower and uses over O(40n) temporary memory.