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