Message ID | 8736huh98l.fsf@oldenburg2.str.redhat.com |
---|---|
State | Committed |
Headers |
Received: (qmail 48716 invoked by alias); 21 Aug 2019 14:41:03 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 48707 invoked by uid 89); 21 Aug 2019 14:41:03 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=HX-Languages-Length:1943 X-HELO: mx1.redhat.com From: Florian Weimer <fweimer@redhat.com> To: libc-alpha@sourceware.org Subject: [PATCH] misc: Use allocate_once in getmntent Date: Wed, 21 Aug 2019 16:40:58 +0200 Message-ID: <8736huh98l.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain |
Commit Message
Florian Weimer
Aug. 21, 2019, 2:40 p.m. UTC
Both the buffer and struct mntent are now allocated on the heap. This results in a slight reduction of RSS usage. 2019-08-21 Florian Weimer <fweimer@redhat.com> * misc/mntent.c (struct mntent_buffer): Define. (mntent_buffer): Adjust type to void *. (allocate): Adjust for allocate_once. (deallocate): New function. (getmntent): Call allocate_once.
Comments
On 21/08/2019 11:40, Florian Weimer wrote: > Both the buffer and struct mntent are now allocated on the heap. > This results in a slight reduction of RSS usage. > > 2019-08-21 Florian Weimer <fweimer@redhat.com> > > * misc/mntent.c (struct mntent_buffer): Define. > (mntent_buffer): Adjust type to void *. > (allocate): Adjust for allocate_once. > (deallocate): New function. > (getmntent): Call allocate_once. > > diff --git a/misc/mntent.c b/misc/mntent.c > index 980ea40967..8fae6064c6 100644 > --- a/misc/mntent.c > +++ b/misc/mntent.c > @@ -18,36 +18,41 @@ > > #include <mntent.h> > #include <stdlib.h> > -#include <libc-lock.h> > +#include <allocate_once.h> > + > +struct mntent_buffer > +{ > + struct mntent m; > + char buffer[4096]; > +}; The struct mntent is just 24 bytes for 32 bits or 40 bytes for 64 bits, I would focus on optimizing the required buffer instead. We need just to read just one line at the, so one option could be to move getmntent_r to misc/mntent.c and add a logic to call getline if the buffer is equal a sentinel value, meaning it was called from getmnent. The getline would be the one responsible to lock the stream and reallocate the buffer if required, allowing having a buffer size with the maximum size actually required. > > /* We don't want to allocate the static buffer all the time since it > - is not always used (in fact, rather infrequently). Accept the > - extra cost of a `malloc'. */ > -libc_freeres_ptr (static char *getmntent_buffer); > - > -/* This is the size of the buffer. This is really big. */ > -#define BUFFER_SIZE 4096 > + is not always used (in fact, rather infrequently). */ > +libc_freeres_ptr (static void *mntent_buffer); > > +static void * > +allocate (void *closure) > +{ > + return malloc (sizeof (struct mntent_buffer)); > +} > > static void > -allocate (void) > +deallocate (void *closure, void *ptr) > { > - getmntent_buffer = (char *) malloc (BUFFER_SIZE); > + free (ptr); > } > > - > struct mntent * > getmntent (FILE *stream) > { > - static struct mntent m; > - __libc_once_define (static, once); > - __libc_once (once, allocate); > - > - if (getmntent_buffer == NULL) > + struct mntent_buffer *buffer = allocate_once (&mntent_buffer, > + allocate, deallocate, NULL); > + if (buffer == NULL) > /* If no core is available we don't have a chance to run the > program successfully and so returning NULL is an acceptable > result. */ > return NULL; > > - return __getmntent_r (stream, &m, getmntent_buffer, BUFFER_SIZE); > + return __getmntent_r (stream, &buffer->m, > + buffer->buffer, sizeof (buffer->buffer)); > } >
* Adhemerval Zanella: > On 21/08/2019 11:40, Florian Weimer wrote: >> Both the buffer and struct mntent are now allocated on the heap. >> This results in a slight reduction of RSS usage. >> >> 2019-08-21 Florian Weimer <fweimer@redhat.com> >> >> * misc/mntent.c (struct mntent_buffer): Define. >> (mntent_buffer): Adjust type to void *. >> (allocate): Adjust for allocate_once. >> (deallocate): New function. >> (getmntent): Call allocate_once. >> >> diff --git a/misc/mntent.c b/misc/mntent.c >> index 980ea40967..8fae6064c6 100644 >> --- a/misc/mntent.c >> +++ b/misc/mntent.c >> @@ -18,36 +18,41 @@ >> >> #include <mntent.h> >> #include <stdlib.h> >> -#include <libc-lock.h> >> +#include <allocate_once.h> >> + >> +struct mntent_buffer >> +{ >> + struct mntent m; >> + char buffer[4096]; >> +}; > > The struct mntent is just 24 bytes for 32 bits or 40 bytes for 64 bits, > I would focus on optimizing the required buffer instead. There are four byte for the __libc_once guard. The main point for doing this is that this storage is always wasted, even if the interface is never called. The change seems simple enough to do now, even if we replace it with something better afterwards (like storing the buffer in the FILE *, or otherwise associating it with it, or making it thread-local). Thanks, Florian
On 22/08/2019 06:06, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 21/08/2019 11:40, Florian Weimer wrote: >>> Both the buffer and struct mntent are now allocated on the heap. >>> This results in a slight reduction of RSS usage. >>> >>> 2019-08-21 Florian Weimer <fweimer@redhat.com> >>> >>> * misc/mntent.c (struct mntent_buffer): Define. >>> (mntent_buffer): Adjust type to void *. >>> (allocate): Adjust for allocate_once. >>> (deallocate): New function. >>> (getmntent): Call allocate_once. >>> >>> diff --git a/misc/mntent.c b/misc/mntent.c >>> index 980ea40967..8fae6064c6 100644 >>> --- a/misc/mntent.c >>> +++ b/misc/mntent.c >>> @@ -18,36 +18,41 @@ >>> >>> #include <mntent.h> >>> #include <stdlib.h> >>> -#include <libc-lock.h> >>> +#include <allocate_once.h> >>> + >>> +struct mntent_buffer >>> +{ >>> + struct mntent m; >>> + char buffer[4096]; >>> +}; >> >> The struct mntent is just 24 bytes for 32 bits or 40 bytes for 64 bits, >> I would focus on optimizing the required buffer instead. > > There are four byte for the __libc_once guard. > > The main point for doing this is that this storage is always wasted, > even if the interface is never called. The change seems simple enough > to do now, even if we replace it with something better afterwards (like > storing the buffer in the FILE *, or otherwise associating it with it, > or making it thread-local). > Right, although a further optimization to reduce buffer would probably require two allocations (on for the mntent_buffer and another for the buffer itself). But I think it should be ok.
* Adhemerval Zanella: > On 22/08/2019 06:06, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> On 21/08/2019 11:40, Florian Weimer wrote: >>>> Both the buffer and struct mntent are now allocated on the heap. >>>> This results in a slight reduction of RSS usage. >>>> >>>> 2019-08-21 Florian Weimer <fweimer@redhat.com> >>>> >>>> * misc/mntent.c (struct mntent_buffer): Define. >>>> (mntent_buffer): Adjust type to void *. >>>> (allocate): Adjust for allocate_once. >>>> (deallocate): New function. >>>> (getmntent): Call allocate_once. >>>> >>>> diff --git a/misc/mntent.c b/misc/mntent.c >>>> index 980ea40967..8fae6064c6 100644 >>>> --- a/misc/mntent.c >>>> +++ b/misc/mntent.c >>>> @@ -18,36 +18,41 @@ >>>> >>>> #include <mntent.h> >>>> #include <stdlib.h> >>>> -#include <libc-lock.h> >>>> +#include <allocate_once.h> >>>> + >>>> +struct mntent_buffer >>>> +{ >>>> + struct mntent m; >>>> + char buffer[4096]; >>>> +}; >>> >>> The struct mntent is just 24 bytes for 32 bits or 40 bytes for 64 bits, >>> I would focus on optimizing the required buffer instead. >> >> There are four byte for the __libc_once guard. >> >> The main point for doing this is that this storage is always wasted, >> even if the interface is never called. The change seems simple enough >> to do now, even if we replace it with something better afterwards (like >> storing the buffer in the FILE *, or otherwise associating it with it, >> or making it thread-local). >> > > Right, although a further optimization to reduce buffer would probably > require two allocations (on for the mntent_buffer and another for the > buffer itself). But I think it should be ok. Sorry, is this a review of the original patch? 8-) Thanks, Florian
On 21/08/2019 11:40, Florian Weimer wrote: > Both the buffer and struct mntent are now allocated on the heap. > This results in a slight reduction of RSS usage. > > 2019-08-21 Florian Weimer <fweimer@redhat.com> > > * misc/mntent.c (struct mntent_buffer): Define. > (mntent_buffer): Adjust type to void *. > (allocate): Adjust for allocate_once. > (deallocate): New function. > (getmntent): Call allocate_once. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > diff --git a/misc/mntent.c b/misc/mntent.c > index 980ea40967..8fae6064c6 100644 > --- a/misc/mntent.c > +++ b/misc/mntent.c > @@ -18,36 +18,41 @@ > > #include <mntent.h> > #include <stdlib.h> > -#include <libc-lock.h> > +#include <allocate_once.h> > + > +struct mntent_buffer > +{ > + struct mntent m; > + char buffer[4096]; > +}; > > /* We don't want to allocate the static buffer all the time since it > - is not always used (in fact, rather infrequently). Accept the > - extra cost of a `malloc'. */ > -libc_freeres_ptr (static char *getmntent_buffer); > - > -/* This is the size of the buffer. This is really big. */ > -#define BUFFER_SIZE 4096 > + is not always used (in fact, rather infrequently). */ > +libc_freeres_ptr (static void *mntent_buffer); > > +static void * > +allocate (void *closure) > +{ > + return malloc (sizeof (struct mntent_buffer)); > +} > > static void > -allocate (void) > +deallocate (void *closure, void *ptr) > { > - getmntent_buffer = (char *) malloc (BUFFER_SIZE); > + free (ptr); > } > > - > struct mntent * > getmntent (FILE *stream) > { > - static struct mntent m; > - __libc_once_define (static, once); > - __libc_once (once, allocate); > - > - if (getmntent_buffer == NULL) > + struct mntent_buffer *buffer = allocate_once (&mntent_buffer, > + allocate, deallocate, NULL); > + if (buffer == NULL) > /* If no core is available we don't have a chance to run the > program successfully and so returning NULL is an acceptable > result. */ > return NULL; > > - return __getmntent_r (stream, &m, getmntent_buffer, BUFFER_SIZE); > + return __getmntent_r (stream, &buffer->m, > + buffer->buffer, sizeof (buffer->buffer)); > } > Ok.
diff --git a/misc/mntent.c b/misc/mntent.c index 980ea40967..8fae6064c6 100644 --- a/misc/mntent.c +++ b/misc/mntent.c @@ -18,36 +18,41 @@ #include <mntent.h> #include <stdlib.h> -#include <libc-lock.h> +#include <allocate_once.h> + +struct mntent_buffer +{ + struct mntent m; + char buffer[4096]; +}; /* We don't want to allocate the static buffer all the time since it - is not always used (in fact, rather infrequently). Accept the - extra cost of a `malloc'. */ -libc_freeres_ptr (static char *getmntent_buffer); - -/* This is the size of the buffer. This is really big. */ -#define BUFFER_SIZE 4096 + is not always used (in fact, rather infrequently). */ +libc_freeres_ptr (static void *mntent_buffer); +static void * +allocate (void *closure) +{ + return malloc (sizeof (struct mntent_buffer)); +} static void -allocate (void) +deallocate (void *closure, void *ptr) { - getmntent_buffer = (char *) malloc (BUFFER_SIZE); + free (ptr); } - struct mntent * getmntent (FILE *stream) { - static struct mntent m; - __libc_once_define (static, once); - __libc_once (once, allocate); - - if (getmntent_buffer == NULL) + struct mntent_buffer *buffer = allocate_once (&mntent_buffer, + allocate, deallocate, NULL); + if (buffer == NULL) /* If no core is available we don't have a chance to run the program successfully and so returning NULL is an acceptable result. */ return NULL; - return __getmntent_r (stream, &m, getmntent_buffer, BUFFER_SIZE); + return __getmntent_r (stream, &buffer->m, + buffer->buffer, sizeof (buffer->buffer)); }