classification
Title: unnecessary copying of memoryview in gzip.GzipFile.write?
Type: performance Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: martin.panter, python-dev, serhiy.storchaka, skrah, vstinner, wolma
Priority: normal Keywords: patch

Created on 2015-03-17 14:26 by wolma, last changed 2015-03-23 14:59 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
memoryview_write.patch wolma, 2015-03-17 14:27 review
test_memoryview_write.patch wolma, 2015-03-17 15:47 review
write_bytes_like_objects.patch wolma, 2015-03-20 13:17 review
write_bytes_like_objects_v2.patch wolma, 2015-03-22 22:21 review
write_bytes_like_objects_v3.patch wolma, 2015-03-23 10:28 review
gzip_write_noncontiguous.patch serhiy.storchaka, 2015-03-23 14:37 review
Messages (31)
msg238294 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-17 14:26
I thought I'd go back to work on a test patch for issue21560 today, but now I'm puzzled by the explicit handling of memoryviews in gzip.GzipFile.write.
The method is defined as:

    def write(self,data):
        self._check_closed()
        if self.mode != WRITE:
            import errno
            raise OSError(errno.EBADF, "write() on read-only GzipFile object")

        if self.fileobj is None:
            raise ValueError("write() on closed GzipFile object")

        # Convert data type if called by io.BufferedWriter.
        if isinstance(data, memoryview):
            data = data.tobytes()

        if len(data) > 0:
            self.size = self.size + len(data)
            self.crc = zlib.crc32(data, self.crc) & 0xffffffff
            self.fileobj.write( self.compress.compress(data) )
            self.offset += len(data)

        return len(data)

So for some reason, when it gets passed data as a meoryview it will first copy its content to a bytes object and I do not understand why.
zlib.crc32 and zlib.compress seem to be able to deal with memoryviews so the only sepcial casing that seems required here is in determining the byte length of the data, which I guess needs to use memoryview.nbytes. I've prepared a patch (overlapping the one for issue21560) that avoids copying the data and seems to work fine.

Did I miss something about the importance of the tobytes conversion ?
msg238295 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 14:32
The patch looks good to be me, but it lacks an unit test. Can you please add a simple unit test to ensure that it's possible to memoryview to write(), and that the result is correct? (ex: uncompress and ensure that you get the same content)
msg238300 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-17 15:19
Better way is data = data.cast('B').
msg238301 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 15:22
> Better way is data = data.cast('B').

Why is this cast required? Can you please elaborate? If some memoryview must be rejected, again, we need more unit tests.
msg238304 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-17 15:47
Here is a patch with memoryview tests.
Are tests and code patches supposed to go in one file or separate ones ?
msg238305 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-17 15:50
@Serhiy:
Why would data = data.cast('B') be required ? When would the memoryview not be in 'B' format already ?
msg238307 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-17 16:05
memoryview is converted to bytes because len() for memoryview returns a size of first dimension (a number of items for one-dimension view), not a number of bytes.

>>> m = memoryview(array.array('I', [1, 2, 3]))
>>> len(m)
3
>>> len(m.tobytes())
12
>>> len(m.cast('B'))
12
msg238308 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-17 16:09
> memoryview is converted to bytes because len() for memoryview returns a size of first dimension (a number of items for one-dimension view), not a number of bytes.
> 
>>>> m = memoryview(array.array('I', [1, 2, 3]))
>>>> len(m)
> 3
>>>> len(m.tobytes())
> 12
>>>> len(m.cast('B'))
> 12

Right, I was aware of this. But are you saying that my proposed solution (using memoryview.nbytes) is wrong ? If so, then cast is certainly an option and should still outperform tobytes.
msg238315 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 16:57
> Are tests and code patches supposed to go in one file or separate ones ?

It's more convinient to have a single patch with both changes.
msg238329 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-17 18:41
You patch is correct Wolfgang, but with cast('B') the patch would be smaller (no need to replace len(data) to nbytes).

While we are here, it is possible to add the support of general byte-like objects.

if not isinstance(data, bytes):
    data = memoryview(data).cast('B')

isinstance() check is just for optimization, it can be omitted if doesn't affect a performance.
msg238344 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 21:43
> While we are here, it is possible to add the support of general byte-like objects.

With and without the patch, write() accepts bytes, bytearray and memoryview. Which other byte-like types do you know?

writeframesraw() method of aifc, sunau and wave modules use this pattern:

    if not isinstance(data, (bytes, bytearray)):
        data = memoryview(data).cast('B')

We can maybe reuse it in gzip module?
msg238350 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-17 22:16
> With and without the patch, write() accepts bytes, bytearray and memoryview.
> Which other byte-like types do you know?

The "bytes-like object" term is used as an alias of "an instance of type that 
supports buffer protocol". Besides bytes, bytearray and memoryview, this is 
array.array and NumPy arrays. file.write() supports arbitrary bytes-like 
objects, including array.array and NumPy arrays.

> writeframesraw() method of aifc, sunau and wave modules use this pattern:

Yes, I wrote this code, if I remember correct.
msg238376 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-18 04:36
I would say that the current patch looks correct enough, in that it would still get the correct lengths when a memoryview() object is passed in. The zlib module’s crc32() function and compress() method already seem to support arbitrary bytes-like objects.

But to make GzipFile.write() also accept arbitrary bytes-like objects, you probably only need to change the code calculating the length to something like:

with memoryview(data) as view:
    length = view.nbytes

# Go on to call compress(data) and crc32(data)
msg238385 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-18 08:52
Thanks everyone for the lively discussion ! 

I like Serhiy's idea of making write work with arbitrary objects supporting the buffer protocol. In fact, I noticed before that GzipFile.write misbehaves with array.array input. It pretends to accept that, but it'll use len(data) for calculating the zip file metadata so reading from the file will later fail. I was assuming then that fixing that would be too complicated for a rather exotic usecase, but now that I see how simple it really is I think it should be done.

As for the concrete implementation, I guess an isinstance(data, bytes) check to speed up treatment of the most common input is a good idea, but I am not convinced that bytearray deserves the same attention.

Regarding memoryview.cast('B') vs memoryview.nbytes, I see Serhiy's point of keeping the patch size smaller. Personally though, I find use of nbytes much more self-explanatory than cast('B') the purpose of which was not immediately obvious to me. So I would opt for better readability of the final code rather than optimizing patch size, but I would be ok with either solution since it is not about the essence of the patch anyway.

Finally, the bug I report in issue21560 would be fixed as a side-effect of this patch here (because trying to get a memoryview from str would throw an early TypeError). Still, I think it would be a good idea to try to write to the wrapped fileobj *before* updating self.size and self.crc to be protected from unforeseen errors. So maybe we could include that change in the patch here ?

With all that the final code section could look like this:

        if isinstance(data, bytes):
            length = len(data)
        else:
            data = memoryview(data)
            length = data.nbytes

        if length > 0:
            self.fileobj.write( self.compress.compress(data) )
            self.size = self.size + length
            self.crc = zlib.crc32(data, self.crc) & 0xffffffff
            self.offset += length

        return length

One remaining detail then would be whether one would want to catch the TypeError possibly raised by the memoryview constructor to turn it into something less confusing (after all many users will not know what a memoryview has to do with all this). The above code would throw (with str input for example):

Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/home/wolma/gzip-bug/Lib/gzip.py", line 340, in write
    data = memoryview(data)
TypeError: memoryview: a bytes-like object is required, not 'str'

Maybe, this could be turned into:

TypeError: must be bytes / bytes-like object, not 'str'  ?

to be consistent with the corresponding error in 'wt' mode ?

Let me know which of the above options you favour and I'll provide a new patch.
msg238666 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-20 13:17
ok, I've prepared a patch including tests based on my last suggestion, which I think is ready for getting reviewed.
msg238946 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-22 22:21
Here is a revised version of my patch addressing Serhiy's review comments.
msg239006 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-23 10:39
In general the patch LGTM.
msg239015 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-23 13:28
New changeset 4dc69e5124f8 by Serhiy Storchaka in branch 'default':
Issue #23688: Added support of arbitrary bytes-like objects and avoided
https://hg.python.org/cpython/rev/4dc69e5124f8
msg239019 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-03-23 13:52
I think there's a behavior change: Before you could gzip non-contiguous
views directly, now that operation raises BufferError.
msg239020 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-23 13:57
Could you provide an example?
msg239024 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-03-23 14:08
Sure:

import gzip
x = memoryview(b'x' * 10)
y = x[::-1]
with gzip.GzipFile("xxxxx", 'w') as f:
    f.write(y)
msg239027 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-23 14:22
ouch. haven't thought of this.

OTOH, just plain io with your example:

with open('xy', 'wb') as f:
    f.write(y)

Traceback (most recent call last):
  File "<pyshell#29>", line 2, in <module>
    f.write(y)
BufferError: memoryview: underlying buffer is not C-contiguous

fails too and after all that's not too surprising.

In a sense, the old behavior was an artefact of silently copying the memoryview to bytes. You never used it *directly*.
But, yes, it is a change in (undocumented) behavior :(
msg239029 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-23 14:29
to preserve compatibility:

there is the memoryview.c_contiguous flag. Maybe we should just check it and if it is False fall back to the old copying behavior ?
msg239030 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-23 14:35
something like:


    def write(self,data):
        self._check_closed()
        if self.mode != WRITE:
            import errno
            raise OSError(errno.EBADF, "write() on read-only GzipFile object")

        if self.fileobj is None:
            raise ValueError("write() on closed GzipFile object")

        if isinstance(data, bytes):
            length = len(data)
        elif isinstance(data, memoryview) and not data.c_contiguous:
            data = data.tobytes()
            length = len(data)
        else:
            # accept any data that supports the buffer protocol
            data = memoryview(data)
            length = data.nbytes

        if length > 0:
            self.fileobj.write(self.compress.compress(data))
            self.size += length
            self.crc = zlib.crc32(data, self.crc) & 0xffffffff
            self.offset += length

        return length
msg239031 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-23 14:37
Here is a patch that restores support on non-contiguous memoryviews.

It would be better to drop support of non-contiguous data, because it worked 
only by accident. Needed support of only bytes-like memoryviews written by 
BufferedWriter.
msg239032 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-03-23 14:38
> In a sense, the old behavior was an artefact of silently copying the memoryview to bytes.

It likely wasn't intentional, but tobytes() *is* used to serialize
weird arrays to their C-contiguous representation (following the
logical structure of the array rather than the physical one).

Since the gzip docs don't help much, I guess the new behavior is
probably okay.
msg239033 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-03-23 14:53
I just see that non-contiguous arrays didn't work in 2.7 either,
so that was probably the original intention.
msg239034 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-23 14:54
Serhiy:

I think I saw that you committed this also to the 2.7 branch, but that would not work since memoryviews do not have the nbytes attribute (they do not seem to have cast either). One would have to calculate the length instead from other properties.
Tests would fail too I think.

If I'm mistaken, then sorry for the noise.
msg239035 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-23 14:58
OK, so left it as is if nobody complains.
msg239036 - (view) Author: Wolfgang Maier (wolma) * Date: 2015-03-23 14:59
I see now that it is just issue21560 that went into 2.7 and that's fine.
As I said: sorry for the noise
msg239037 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-23 14:59
> I think I saw that you committed this also to the 2.7 branch,

I committed only working tests and a fix from issue21560.
History
Date User Action Args
2015-03-23 14:59:32serhiy.storchakasetmessages: + msg239037
2015-03-23 14:59:15wolmasetmessages: + msg239036
2015-03-23 14:58:02serhiy.storchakasetmessages: + msg239035
2015-03-23 14:54:06wolmasetmessages: + msg239034
2015-03-23 14:53:53skrahsetmessages: + msg239033
2015-03-23 14:38:53skrahsetmessages: + msg239032
2015-03-23 14:37:02serhiy.storchakasetfiles: + gzip_write_noncontiguous.patch

messages: + msg239031
2015-03-23 14:35:30wolmasetmessages: + msg239030
2015-03-23 14:29:38wolmasetmessages: + msg239029
2015-03-23 14:22:33wolmasetmessages: + msg239027
2015-03-23 14:08:05skrahsetmessages: + msg239024
2015-03-23 13:57:00serhiy.storchakasetmessages: + msg239020
2015-03-23 13:52:21skrahsetnosy: + skrah
messages: + msg239019
2015-03-23 13:32:26serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2015-03-23 13:28:05python-devsetnosy: + python-dev
messages: + msg239015
2015-03-23 10:39:53serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg239006
stage: commit review
2015-03-23 10:28:12wolmasetfiles: + write_bytes_like_objects_v3.patch
2015-03-22 22:21:45wolmasetfiles: + write_bytes_like_objects_v2.patch

messages: + msg238946
2015-03-20 13:17:41wolmasetfiles: + write_bytes_like_objects.patch

messages: + msg238666
2015-03-18 08:52:18wolmasetmessages: + msg238385
2015-03-18 04:36:55martin.pantersetnosy: + martin.panter
messages: + msg238376
2015-03-17 22:16:10serhiy.storchakasetmessages: + msg238350
title: unnecessary copying of memoryview in gzip.GzipFile.write ? -> unnecessary copying of memoryview in gzip.GzipFile.write?
2015-03-17 21:43:48vstinnersetmessages: + msg238344
2015-03-17 18:41:39serhiy.storchakasetmessages: + msg238329
2015-03-17 16:57:04vstinnersetmessages: + msg238315
2015-03-17 16:09:58wolmasetmessages: + msg238308
2015-03-17 16:05:29serhiy.storchakasetmessages: + msg238307
2015-03-17 15:50:31wolmasetmessages: + msg238305
2015-03-17 15:47:16wolmasetfiles: + test_memoryview_write.patch

messages: + msg238304
2015-03-17 15:22:54vstinnersetmessages: + msg238301
2015-03-17 15:19:52serhiy.storchakasetmessages: + msg238300
2015-03-17 14:33:11vstinnersetnosy: + serhiy.storchaka
type: resource usage -> performance
2015-03-17 14:32:59vstinnersetnosy: + vstinner
messages: + msg238295
2015-03-17 14:27:07wolmasetfiles: + memoryview_write.patch
keywords: + patch
2015-03-17 14:26:04wolmacreate