diff mbox

misc: Use allocate_once in getmntent

Message ID 8736huh98l.fsf@oldenburg2.str.redhat.com
State Committed
Headers

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

Adhemerval Zanella Aug. 21, 2019, 8:19 p.m. UTC | #1 
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));
>  }
>
Florian Weimer Aug. 22, 2019, 9:06 a.m. UTC | #2 
* 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
Adhemerval Zanella Aug. 27, 2019, 1:29 p.m. UTC | #3 
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.
Florian Weimer Aug. 27, 2019, 1:32 p.m. UTC | #4 
* 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
Adhemerval Zanella Aug. 27, 2019, 1:33 p.m. UTC | #5 
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 mbox

Patch

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));
 }