unnecessary copying of memoryview in gzip.GzipFile.write?

[wm75]

BPO	23688
Nosy	@vstinner, @skrah, @vadmium, @serhiy-storchaka, @wm75
Files	memoryview_write.patch test_memoryview_write.patch write_bytes_like_objects.patch write_bytes_like_objects_v2.patch write_bytes_like_objects_v3.patch gzip_write_noncontiguous.patch

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-03-23.13:32:26.028>
created_at = <Date 2015-03-17.14:26:04.343>
labels = ['library', 'performance']
title = 'unnecessary copying of memoryview in gzip.GzipFile.write?'
updated_at = <Date 2015-03-23.14:59:32.704>
user = 'https://github.com/wm75'

bugs.python.org fields:

activity = <Date 2015-03-23.14:59:32.704>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-03-23.13:32:26.028>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2015-03-17.14:26:04.343>
creator = 'wolma'
dependencies = []
files = ['38521', '38526', '38600', '38639', '38650', '38656']
hgrepos = []
issue_num = 23688
keywords = ['patch']
message_count = 31.0
messages = ['238294', '238295', '238300', '238301', '238304', '238305', '238307', '238308', '238315', '238329', '238344', '238350', '238376', '238385', '238666', '238946', '239006', '239015', '239019', '239020', '239024', '239027', '239029', '239030', '239031', '239032', '239033', '239034', '239035', '239036', '239037']
nosy_count = 6.0
nosy_names = ['vstinner', 'skrah', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'wolma']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue23688'
versions = ['Python 3.5']

[wm75]

I thought I'd go back to work on a test patch for bpo-21560 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 bpo-21560) that avoids copying the data and seems to work fine.

Did I miss something about the importance of the tobytes conversion ?

[vstinner]

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)

[serhiy-storchaka]

Better way is data = data.cast('B').

[vstinner]

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.

[wm75]

Here is a patch with memoryview tests.
Are tests and code patches supposed to go in one file or separate ones ?

[wm75]

@serhiy:
Why would data = data.cast('B') be required ? When would the memoryview not be in 'B' format already ?

[serhiy-storchaka]

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

[wm75]

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.

[vstinner]

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.

[serhiy-storchaka]

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.

[vstinner]

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?