Repository navigation
Add _PyBytesWriter API #61942
Description
| BPO | 17742 |
|---|---|
| Nosy | @pitrou, @vstinner, @bitdancer, @serhiy-storchaka |
| Files |
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:
assignee = None
closed_at = <Date 2013-05-16.21:26:58.896>
created_at = <Date 2013-04-15.22:03:35.251>
labels = ['interpreter-core', 'type-feature', 'invalid']
title = 'Add _PyBytesWriter API'
updated_at = <Date 2013-05-16.21:26:58.895>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2013-05-16.21:26:58.895>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2013-05-16.21:26:58.896>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2013-04-15.22:03:35.251>
creator = 'vstinner'
dependencies = []
files = ['29877', '29878', '29914', '29928', '29931', '29961']
hgrepos = []
issue_num = 17742
keywords = ['patch']
message_count = 18.0
messages = ['187035', '187036', '187053', '187057', '187217', '187263', '187274', '187297', '187304', '187384', '187476', '187478', '187480', '187484', '187522', '187595', '187944', '189415']
nosy_count = 4.0
nosy_names = ['pitrou', 'vstinner', 'r.david.murray', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue17742'
versions = ['Python 3.4']
Activity
vstinner commentedon Apr 15, 2013
In Python 3.3, I added _PyUnicodeWriter API to factorize code handling a Unicode "buffer", just the code to allocate memory and resize the buffer if needed.
I propose to do the same with a new _PyBytesWriter API. The API is very similar to _PyUnicodeWriter:
The patch changes ASCII, Latin1, UTF-8 and charmap encoders to use _PyBytesWriter API. A second patch changes CJK encoders.
I did not run a benchmark yet. I wrote a patch to factorize the code, not the make the code faster.
Notes on performances:
I peek the "small buffer allocated on the stack" idea from UTF-8 encoder, but the smaller buffer is always 500 bytes (instead of a size depending on the Unicode maximum character of the input Unicode string)
_PyBytesWriter overallocates by 25% (when overallocation is enabled), whereas charmap encoders doubles the buffer:
/* exponentially overallocate to minimize reallocations /
if (requiredsize < 2outsize)
requiredsize = 2*outsize;
I didn't check if the allocation size is the same with the patch. min_size and overallocate attributes should be set correctly to not make the code slower.
The code writing a single into a _PyUnicodeWriter buffer is inlined in unicodeobject.c. _PyBytesWriter API does not provide inlined function for the same purpose.
vstinner commentedon Apr 15, 2013
See also bpo-17694.
serhiy-storchaka commentedon Apr 16, 2013
Could you please provide the benchmarks results? I am afraid that it may hit a performance. Results on Windows are especially interesting.
pitrou commentedon Apr 16, 2013
Your patches don't seem to reduce the line count, so I don't understand the point.
vstinner commentedon Apr 18, 2013
Here is a benchmark. It looks like the overallocation is not configured correctly for charmap and UTF-8 encoders: it is always enabled for UTF-8 encoder (whereas it should only be enabled on the first call to an error handler), and never enabled for charmap encoder.
serhiy-storchaka commentedon Apr 18, 2013
I run my own benchmarks and don't see any regression besides a random noise. Actually I see even speed up for some cases:
./python -m timeit -s "a = '\x80'*10000" "a.encode()"
Before patch: 29.8 usec per loop, after patch: 21.5 usec per loop.
This is just a compiler's caprice.
serhiy-storchaka commentedon Apr 18, 2013
I have added some comments on Rietveld.
vstinner commentedon Apr 18, 2013
Benchmark script, should be used with:
https://bitbucket.org/haypo/misc/src/tip/python/benchmark.py
vstinner commentedon Apr 18, 2013
New version of the patch:
Performances are almost the same than without the patch. It looks like they are a little bit better.
serhiy-storchaka commentedon Apr 19, 2013
_PyBytesWriter and _PyUnicodeWriter have differen use cases. While _PyUnicodeWriter used primary in formatter where resulting size is rarely known and reallocation in decoders usually caused by widening result, _PyBytesWriter is used only in decoders where we usually can estimate a result size or it's upper bound. Resizing happened only in exceptional cases, when error handler called.
The patch contains a special case for writing only one bytes object. This is very unlikely case. It happened only when an encoded string contains only one illegal character. I think this case is not worth a complication and
obfuscation of the code. I think we should drop readonly attribute and a piece of the code (which looks buggy for me anyway).
vstinner commentedon Apr 20, 2013
The patch only modify a few functions to make them use the new _PyBytesWriter API. Other functions can use it.
A few examples:
But I agree that the readonly "hack" can be removed from _PyBytesWriter API since the bytes type has no format method (no bytes%args nor bytes.format(args)).
bitdancer commentedon Apr 21, 2013
It may eventually get one, though.
vstinner commentedon Apr 21, 2013
If a use case for the "read-only hack" comes, the hack can be added again later. It's better to start with something simple and extend it with new use cases.
vstinner commentedon Apr 21, 2013
I'm not completly satisfied of bytes_writer-2.patch. Most encoders work directly on a pointer (char*). If I want to keep the code using pointers (because it is efficient), I have to resynchronize the writer and the pointer before and after calling writer methods.
Here is a new patch implementing a different approach, closer to the current code. The patch version 3 has no "pos" attribute: the "position" is now a pointer (str). So encoders can just use this pointer instead of their own pointer.
I expect that replacing "*p++ = c;" with "*writer.str++ = c;" would not add an important overhead, especially because writer is a local variable, and str is the first attribute of the structure. I hope that the machine code will be exactly the same.
pitrou commentedon Apr 21, 2013
The last patch increases the size of the code substantially. I'm still wondering what the benefits are.
Objects/unicodeobject.c | 267 ++++++++++++++++-------------------------
5 files changed, 368 insertions(+), 245 deletions(-)
vstinner commentedon Apr 22, 2013
"I expect that replacing "*p++ = c;" with "*writer.str++ = c;" would
not add an important overhead, especially because writer is a local
variable, and str is the first attribute of the structure. I hope that
the machine code will be exactly the same."
I ran some benchmarks: performances are better in some cases, but
worse in other cases. Performances are worse for simple loop like:
For the "writer.str++" instruction, the new pointer value is written
immediatly in the structure. The pointer value is also read again at
each iteration. So we have 1 load and 1 store per iteration. I read
the assembler code on x86_64 using GCC 4.2.1 of Mac OS 10.6.
I compared with the original code:
GCC emits better code: str is stored in a register and the new value
of str is only written once, at the end of loop (instead of writing it
at each iteration). The pointer value is read before the loop. So we
have 0 load and 0 store (related to the pointer value) in the body of
the loop.
It may be an aliasing issue, but I didn't find how to say to GCC that
the new value of writer.str can be written only once at the end of the
loop. I tried to add __restrict__ keyword: the load (get the pointer
value) is moved out of the loop. But the store is still in the body of
the loop.
vstinner commentedon Apr 27, 2013
Advantages of the patch.
finer control on how the buffer is allocated: only overallocate if the replacement string (while handling an encoding error) is longer than 1 byte/character. The "replace" error handler should never use overallocation for example. Overallocation (when misused, when it was not needed) has a cost at the end of the encoder, because the buffer must be resized (shrink)
use a buffer allocated on the stack for short strings. I'm not really convinced of this optimization. The data is still copied when the result is converted to a bytes objects (PyBytes_FromStringAndSize). It may be interesting if the encoder has to handle one or more errors: no need to resize the buffer until we reach the size of the small buffer (ex: 512 bytes).
handle correctly integer overflow: most encoders do not catch integer overflow errors and may fail to handle (very) long strings (ex: encoded string longer than PY_SSIZE_T_MAX).
I'm not convinced that the patch would permit to design faster code. According to the assembler, it is the opposite (when "*writer.str++" is used in a loop). I don't know if it's possible to design a more efficient _PyBytesWriter API (to help GCC to generate more efficient machine code), nor if the overhead is important in a "normal case" (bench_encoders.py tests border cases, text with many many errors).
vstinner commentedon May 16, 2013
_PyBytesWriter API makes the code slower and does not really reduce the number of lines, so I'm closing this issue as invalid.