[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

_PyBytesWriter API makes the code slower and does not really reduce the number 
of lines, so I'm closing this issue as invalid.

--
resolution:  -> invalid
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

Changes by STINNER Victor :


--
Removed message: http://bugs.python.org/msg187943

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

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 
character. The "replace" error handler should never use overallocation for 
example. Overallocation 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 read 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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

"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:

while (collstart++

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[Antoine Pitrou]

Antoine Pitrou added the comment:

The last patch increases the size of the code substantially. I'm still 
wondering what the benefits are.

$ hg di --stat
 Include/bytesobject.h  |   90 ++
 Misc/NEWS  |3 +
 Objects/bytesobject.c  |  144 ++
 Objects/stringlib/codecs.h |  109 
 Objects/unicodeobject.c|  267 -
 5 files changed, 368 insertions(+), 245 deletions(-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

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.

--
Added file: http://bugs.python.org/file29961/bytes_writer-3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

> It may eventually get one, though.

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[R. David Murray]

R. David Murray added the comment:

It may eventually get one, though.

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

> The patch contains a special case for writing only one bytes object.
> This is very unlikely case.

The patch only modify a few functions to make them use the new _PyBytesWriter 
API. Other functions can use it.

A few examples:

 - PyBytes_FromObject()
 - binascii: binascii_rledecode_hqx()
 - bz2, lzma and zlib modules
 - marshal and pickle modules
 - datetime.datetime.strftime()
 - Python/compile.c: assemble_lnotab()
 - more Unicode decoders

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)).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

_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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

New version of the patch:
 - address most (all?) Serhiy's remarks
 - _PyBytesWriter_PrepareInternal() always use min_size, not only when 
overallocate is 1
 - add more comments

Performances are almost the same than without the patch. It looks like they are 
a little bit better.

--
Added file: http://bugs.python.org/file29931/bytes_writer-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

Benchmark script, should be used with:
https://bitbucket.org/haypo/misc/src/tip/python/benchmark.py

--
Added file: http://bugs.python.org/file29928/bench_encoders.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I have added some comments on Rietveld.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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'*1"  "a.encode()"

Before patch: 29.8 usec per loop, after patch: 21.5 usec per loop.
This is just a compiler's caprice.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

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.

--
Added file: http://bugs.python.org/file29914/bench_encoders.compare

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[Antoine Pitrou]

Antoine Pitrou added the comment:

> I did not run a benchmark yet. I wrote a patch to factorize the code, 
> not the make the code faster.

Your patches don't seem to reduce the line count, so I don't understand the 
point.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
nosy: +pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Could you please provide the benchmarks results? I am afraid that it may hit a 
performance. Results on Windows are especially interesting.

--
components: +Interpreter Core
stage:  -> patch review
type:  -> enhancement

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

STINNER Victor added the comment:

See also #17694.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

Changes by STINNER Victor :


Added file: http://bugs.python.org/file29878/bytes_writer_cjkcodecs.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17742] Add _PyBytesWriter API

[STINNER Victor]

New submission from STINNER Victor:

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:

 * _PyBytesWriter_Init(writer)
 * _PyBytesWriter_Prepare(writer, count)
 * _PyBytesWriter_WriteStr(writer, bytes_obj)
 * _PyBytesWriter_WriteChar(writer, ch)
 * _PyBytesWriter_Finish(writer)
 * _PyBytesWriter_Dealloc(writer)

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 < 2*outsize)
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.

--
files: bytes_writer.patch
keywords: patch
messages: 187035
nosy: haypo, serhiy.storchaka
priority: normal
severity: normal
status: open
title: Add _PyBytesWriter API
versions: Python 3.4
Added file: http://bugs.python.org/file29877/bytes_writer.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com