Repository navigation
Conversation
Replace soft deprecated _PyBytes_Resize() with PyBytesWriter.
|
I marked the PR as a draft since benchmarks should be run on this change.
See also PR gh-139976 which modified _bz2, _lzma, _zstd and zlib extensions to use PyBytesWriter (in |
|
@emmatyping: Do you have advices on what I should measure to check that the change doesn't introduce performance regression? Do you have maybe an existing benchmark? |
|
With #161478 I'm thinking of doing a deeper dive into why the behavior is different between glibc Linux, macOS, and Windows for PyBytesWriter. I'd like to fix this in PyBytesWriter so that is performant across all platforms. I'll follow up here with findings/info. |
|
I just made a comment in referenced issue. Performance is better on all platforms. Just only in situations were the user actually cares about performance and chooses small cache-local output blocks and streaming. Is performance important when the user does not care/know about how detrimental (de)compressing everything into memory at once is? Regrowing an in-memory buffer has a trade-off. Either the allocated buffer fits, and it can be repurposed to a bytes object and thus is very fast. Or it doesn't and realloc is called. In that case the memory might need to be moved requiring a memcpy call. The problem on multiple reallocations is that this requires multiple memcpy calls, and the same bit of memory at the beginning of the buffer is copied multiple times. That is a waste. The previous block allocation mechanism prevented this by filling several blocks which did not need to be resized and then memcpy'ing them into the output. This means all filled op memory is only memcpy'ed once. This is a lot faster for bigger in-memory outputs. The disadvantage is the overhead of such a system, which makes it slower for smaller outputs. For python-isal (zlib/gzip-compatible library, but faster) I choose the growth of in-memory buffer because little overhead for small sizes rewards good cpu-cache local use of small buffers and I can assume that when you are using python-isal, you actually care about that. For CPython, I am not so sure. Python is a beginner-friendly language, so I assume many people will do the easy thing, rather than the more convoluted thing that is actually better for the cpu cache. |
|
This PR changes the allocation strategy: replacing If we are shy and don't want to take any risk, I can just use the private API and set |
Yeah, I saw that issue and I'm concerned about the (unexpected) worse performance. I agree that if a solution is designed, it would be better to implement it directly in PyBytesWriter, so all PyBytesWriter users would benefit from it. (Well, let's continue the discussion in #161478.) |
|
@vstinner, I already stumbled upon this issue with the final What helped in my tests is keeping the block in |
Replace soft deprecated _PyBytes_Resize() with PyBytesWriter.