[PATCH] misc: Use allocate_once in getmntent

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] misc: Use allocate_once in getmntent

Florian Weimer-5
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  <[hidden email]>

        * 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];
+};
 
 /* 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));
 }
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] misc: Use allocate_once in getmntent

Adhemerval Zanella-2


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  <[hidden email]>
>
> * 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));
>  }
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] misc: Use allocate_once in getmntent

Florian Weimer-5
* 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  <[hidden email]>
>>
>> * 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] misc: Use allocate_once in getmntent

Adhemerval Zanella-2


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  <[hidden email]>
>>>
>>> * 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] misc: Use allocate_once in getmntent

Florian Weimer-5
* 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  <[hidden email]>
>>>>
>>>> * 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] misc: Use allocate_once in getmntent

Adhemerval Zanella-2
In reply to this post by Florian Weimer-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  <[hidden email]>
>
> * 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 <[hidden email]>

>
> 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.