[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH] qcow2: do not allocate extra memory
Date: Tue, 12 Jul 2016 14:30:34 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

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: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]