From patchwork Fri Nov 27 15:03:19 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Carlos O'Donell X-Patchwork-Id: 549495 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 24DCD14031C for ; Sat, 28 Nov 2015 02:03:34 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b=FKVe9HOT; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:to:from:subject:message-id:date:mime-version :content-type:content-transfer-encoding; q=dns; s=default; b=PYc exyDwx65ZBpfN+tMVi7eykuQ1XFlM6MSBQL52OWXuuMq7wxFQ4yQinahMlAmWNIL hADvLjiZvkyOAg5e7bhMEiZHa83ud8hveSU4nACrKen1JZESHq04WBIbxuhklWCL ly3W6x00MISANQIc+MZjG/ai460UYdkNbU5aBvZI= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:to:from:subject:message-id:date:mime-version :content-type:content-transfer-encoding; s=default; bh=2xRWPOxsM woWQDqpO6Z4cIuOQWc=; b=FKVe9HOTyZowJi5Nf2urxi9W5qswy2lqR0QuBrOOE D/60dXh2g8AoxHOJp8ibqLeucx0MnuIqLm7i+re8CtBfJxCT9HPCEvp98G93IAh5 BTcaJy48vrx/Qp/8XctzGWgh5kgTuc7fNqNl+sQsWkMzWn5/7Qge+ecuckIHNXyj HQ= Received: (qmail 121430 invoked by alias); 27 Nov 2015 15:03:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 121395 invoked by uid 89); 27 Nov 2015 15:03:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com To: GNU C Library , Siddhesh Poyarekar From: "Carlos O'Donell" Subject: [PATCH] libio/vasprintf.c: Always use realloc. X-Enigmail-Draft-Status: N1110 Message-ID: <565870B7.2020901@redhat.com> Date: Fri, 27 Nov 2015 10:03:19 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 The asprintf/vasprintf implementations calls into _IO_vasprintf to carry out operations on a string-based FILE* object. When the work is complete and the buffer filled with the result the implementation null-terminates and returns the string which is passed back to the caller of asprintf. During underflow the string-based buffer might have grown, and conversely the buffer might be exactly the right size, but short by 1-byte for the NULL terminator. Therefore the implementation chooses to realloc the buffer to return back any excess memory or get 1-byte more for the NULL terminator. In 2000 the implementation was changed by Ulrich Drpper to make a local deicsion to use realloc vs. malloc/memcpy/free based on a heuristic and in order to reduce memory fragmentation. The idea is that if the buffer is only just bigger than what the user needs the realloc will split the chunk and increase fragmentation. While this is true, this decision should not be made at the _IO_vasprintf level. The allocator should be making this decision based on known fragmentation estimates and a tunable fragmentation parameter which could be used to control when realloc splits a chunk and when it decides to malloc/memcpy/free. Therefore we remove the local heuristic. This should increase performance slightly at the cost of fragmentation. Future malloc work to handle fragmentation should be pursued. If someone measures fragmentation and tracks it back down to _IO_vasprintf, we add a comment to guide the reader to what we consider is best practice: enhance realloc to handle fragmentation decisions. OK to checkin? No regressions on x86_64. Cheers, Carlos. 2015-11-27 Carlos O'Donell * libio/vasprintf.c (_IO_vasprintf): Always use realloc. diff --git a/libio/vasprintf.c b/libio/vasprintf.c index 61cdfdd..5b39ee7 100644 --- a/libio/vasprintf.c +++ b/libio/vasprintf.c @@ -41,7 +41,6 @@ _IO_vasprintf (char **result_ptr, const char *format, _IO_va_list args) _IO_strfile sf; int ret; _IO_size_t needed; - _IO_size_t allocated; /* No need to clear the memory here (unlike for open_memstream) since we know we will never seek on the stream. */ string = (char *) malloc (init_string_size); @@ -62,24 +61,18 @@ _IO_vasprintf (char **result_ptr, const char *format, _IO_va_list args) free (sf._sbf._f._IO_buf_base); return ret; } - /* Only use realloc if the size we need is of the same (binary) - order of magnitude then the memory we allocated. */ needed = sf._sbf._f._IO_write_ptr - sf._sbf._f._IO_write_base + 1; - allocated = sf._sbf._f._IO_write_end - sf._sbf._f._IO_write_base; - if ((allocated >> 1) <= needed) - *result_ptr = (char *) realloc (sf._sbf._f._IO_buf_base, needed); - else - { - *result_ptr = (char *) malloc (needed); - if (*result_ptr != NULL) - { - memcpy (*result_ptr, sf._sbf._f._IO_buf_base, needed - 1); - free (sf._sbf._f._IO_buf_base); - } - else - /* We have no choice, use the buffer we already have. */ - *result_ptr = (char *) realloc (sf._sbf._f._IO_buf_base, needed); - } + /* We are either shrinking the buffer, doing nothing, or growing + the buffer by 1 byte for the NULL. In all of these cases it's + fastest to call realloc. At the cost of more memory we could + choose not to realloc and return a larger buffer, but tuning + that requires some thought. At one point this code was expanded + to consider realloc vs. malloc/memcpy/free as a method for + reducing fragmentation caused by realloc, but was later removed + because such decisions are tuning parameters that should be part + of the realloc decisions made by malloc e.g. fragmentation vs. + making a copy. */ + *result_ptr = (char *) realloc (sf._sbf._f._IO_buf_base, needed); if (*result_ptr == NULL) *result_ptr = sf._sbf._f._IO_buf_base; (*result_ptr)[needed - 1] = '\0';