Re: [Qemu-devel] [PATCH] qcow2: do not allocate extra memory

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

On 07/12/2016 01:11 PM, Vladimir Sementsov-Ogievskiy wrote:
> On 12.07.2016 21:43, Eric Blake wrote:
>> On 07/12/2016 11:43 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> There are no needs to allocate more than one cluster, as we set
>>> avail_out for deflate to one cluster.
>>>

>>> ...
>>> strm.avail_out = s->cluster_size;
>>> strm.next_out = out_buf;
>>>
>>> ret = deflate(&strm, Z_FINISH);
>>> ...
>>> out_len = strm.next_out - out_buf;
>> You've skipped what is done with ret, which will be different according
>> to whether the entire compressed stream fit in the buffer described by
>> strm, and that would have to be audited as part of your proposed patch.
> 
> ret would be Z_STREAM_END if it fit in and Z_OK if not. (if there are no
> errors ofcourse). What I've skipped? I just say that nobody knows about
> this extra allocation - neither zlib nor other code in this function
> (except g_free=).

Okay, I've thought about this a bit more, and chatted with John on IRC.
 It looks like the slop is indeed wasted.  And while I don't know that
performance will be noticeably better, I _do_ think you are correct that
readability is easier to understand without the slop.

And who knows - for a malloc() implementation that uses mmap for large
requests, and rounds requests up to page multiples, malloc(64k) may
indeed be a more efficient use of memory than malloc(64k+slop), which
has to burn an entire page for memory that is never touched.

So my end conclusion is that I'd like the commit message to be a bit
more comprehensive (include some of your arguments made in the follow up
messages, such as the fact that we correctly handle Z_STREAM_END vs.
Z_OK in deciding whether to go with a compressed cluster in the first
place), but the idea itself is sane.

I'll give R-b to a v2, but not right now, because I want to make sure
the final commit message is sufficient to avoid another hour of digging
through RFC and zlib documentation when it gets revisited down the road.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]