[PATCH] remove unnecessary calls to __atexit_register_cleanup

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

[PATCH] remove unnecessary calls to __atexit_register_cleanup

enh
__sinit unconditionally calls __atexit_register_cleanup.    

Index: stdio/makebuf.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/makebuf.c,v
retrieving revision 1.8
diff -u -r1.8 makebuf.c
--- stdio/makebuf.c 28 Dec 2005 18:50:22 -0000 1.8
+++ stdio/makebuf.c 3 Nov 2014 20:21:15 -0000
@@ -65,7 +65,6 @@
  fp->_bf._size = 1;
  return;
  }
- __atexit_register_cleanup(_cleanup);
  flags |= __SMBF;
  fp->_bf._base = fp->_p = p;
  fp->_bf._size = size;
Index: stdio/setvbuf.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/setvbuf.c,v
retrieving revision 1.11
diff -u -r1.11 setvbuf.c
--- stdio/setvbuf.c 9 Nov 2009 00:18:27 -0000 1.11
+++ stdio/setvbuf.c 3 Nov 2014 20:21:15 -0000
@@ -124,8 +124,7 @@
  flags |= __SNPT;
 
  /*
- * Fix up the FILE fields, and set __cleanup for output flush on
- * exit (since we are buffered in some way).
+ * Fix up the FILE fields.
  */
  if (mode == _IOLBF)
  flags |= __SLBF;
@@ -148,7 +147,6 @@
  fp->_w = 0;
  }
  FUNLOCKFILE(fp);
- __atexit_register_cleanup(_cleanup);
 
  return (ret);
 }

enh
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remove unnecessary calls to __atexit_register_cleanup

enh
ping?

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remove unnecessary calls to __atexit_register_cleanup

Philip Guenther-2
In reply to this post by enh
On Mon, Nov 3, 2014 at 12:24 PM, Elliott Hughes <[hidden email]> wrote:

> __sinit unconditionally calls __atexit_register_cleanup.
>
> --- stdio/makebuf.c     28 Dec 2005 18:50:22 -0000      1.8
> +++ stdio/makebuf.c     3 Nov 2014 20:21:15 -0000
> @@ -65,7 +65,6 @@
>                 fp->_bf._size = 1;
>                 return;
>         }
> -       __atexit_register_cleanup(_cleanup);
>         flags |= __SMBF;
>         fp->_bf._base = fp->_p = p;
>         fp->_bf._size = size;

Yes, all the places that call __smakebuf() call __sinit() first.

[Side note: diffs are better with the -p option.]


> --- stdio/setvbuf.c     9 Nov 2009 00:18:27 -0000       1.11
> +++ stdio/setvbuf.c     3 Nov 2014 20:21:15 -0000
> @@ -124,8 +124,7 @@
>                 flags |= __SNPT;
>
>         /*
> -        * Fix up the FILE fields, and set __cleanup for output flush on
> -        * exit (since we are buffered in some way).
> +        * Fix up the FILE fields.
>          */
>         if (mode == _IOLBF)
>                 flags |= __SLBF;
> @@ -148,7 +147,6 @@
>                 fp->_w = 0;
>         }
>         FUNLOCKFILE(fp);
> -       __atexit_register_cleanup(_cleanup);
>
>         return (ret);
>  }

I'm not convinced on this one.  Does this break a program that sets
the output to buffered and then doesn't write enough to cause a flush?
 e.g.:

#include <stdio.h>
char buf[BUFSIZ];
int
main(void)
{
        setvbuf(stdout, buf, _IOLBF, sizeof buf);
        putchar('@');
        return 0;
}

That should output just an '@', but the setvbuf.c diff breaks that.

Maybe it would be better to do
        if (!__sdidinit)
                __sinit();

like other places?


Philip Guenther

enh
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remove unnecessary calls to __atexit_register_cleanup

enh
On Tue, Nov 18, 2014 at 10:01 PM, Philip Guenther <[hidden email]> wrote:

> On Mon, Nov 3, 2014 at 12:24 PM, Elliott Hughes <[hidden email]> wrote:
>> __sinit unconditionally calls __atexit_register_cleanup.
>>
>> --- stdio/makebuf.c     28 Dec 2005 18:50:22 -0000      1.8
>> +++ stdio/makebuf.c     3 Nov 2014 20:21:15 -0000
>> @@ -65,7 +65,6 @@
>>                 fp->_bf._size = 1;
>>                 return;
>>         }
>> -       __atexit_register_cleanup(_cleanup);
>>         flags |= __SMBF;
>>         fp->_bf._base = fp->_p = p;
>>         fp->_bf._size = size;
>
> Yes, all the places that call __smakebuf() call __sinit() first.
>
> [Side note: diffs are better with the -p option.]
>
>
>> --- stdio/setvbuf.c     9 Nov 2009 00:18:27 -0000       1.11
>> +++ stdio/setvbuf.c     3 Nov 2014 20:21:15 -0000
>> @@ -124,8 +124,7 @@
>>                 flags |= __SNPT;
>>
>>         /*
>> -        * Fix up the FILE fields, and set __cleanup for output flush on
>> -        * exit (since we are buffered in some way).
>> +        * Fix up the FILE fields.
>>          */
>>         if (mode == _IOLBF)
>>                 flags |= __SLBF;
>> @@ -148,7 +147,6 @@
>>                 fp->_w = 0;
>>         }
>>         FUNLOCKFILE(fp);
>> -       __atexit_register_cleanup(_cleanup);
>>
>>         return (ret);
>>  }
>
> I'm not convinced on this one.  Does this break a program that sets
> the output to buffered and then doesn't write enough to cause a flush?
>  e.g.:
>
> #include <stdio.h>
> char buf[BUFSIZ];
> int
> main(void)
> {
>         setvbuf(stdout, buf, _IOLBF, sizeof buf);
>         putchar('@');
>         return 0;
> }
>
> That should output just an '@', but the setvbuf.c diff breaks that.
>
> Maybe it would be better to do
>         if (!__sdidinit)
>                 __sinit();
>
> like other places?

good point. i hadn't considered that stdin/stdout/stderr aren't
arrived at via __sfp.

adding the __sinit stanza here would match the other places it
currently exists: places where buffers are filled/invalidated.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remove unnecessary calls to __atexit_register_cleanup

Philip Guenther-2
On Tue, 18 Nov 2014, enh wrote:
> On Tue, Nov 18, 2014 at 10:01 PM, Philip Guenther <[hidden email]> wrote:
...

> > Maybe it would be better to do
> >         if (!__sdidinit)
> >                 __sinit();
> >
> > like other places?
>
> good point. i hadn't considered that stdin/stdout/stderr aren't
> arrived at via __sfp.
>
> adding the __sinit stanza here would match the other places it currently
> exists: places where buffers are filled/invalidated.

This passes my test program above; oks?


Philip


Index: makebuf.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/makebuf.c,v
retrieving revision 1.8
diff -u -p -r1.8 makebuf.c
--- makebuf.c 28 Dec 2005 18:50:22 -0000 1.8
+++ makebuf.c 19 Nov 2014 07:00:53 -0000
@@ -65,7 +65,6 @@ __smakebuf(FILE *fp)
  fp->_bf._size = 1;
  return;
  }
- __atexit_register_cleanup(_cleanup);
  flags |= __SMBF;
  fp->_bf._base = fp->_p = p;
  fp->_bf._size = size;
Index: setvbuf.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/setvbuf.c,v
retrieving revision 1.11
diff -u -p -r1.11 setvbuf.c
--- setvbuf.c 9 Nov 2009 00:18:27 -0000 1.11
+++ setvbuf.c 19 Nov 2014 07:00:53 -0000
@@ -115,6 +115,13 @@ nbf:
  }
 
  /*
+ * We're committed to buffering from here, so make sure we've
+ * registered to flush buffers on exit.
+ */
+ if (!__sdidinit)
+ __sinit();
+
+ /*
  * Kill any seek optimization if the buffer is not the
  * right size.
  *
@@ -148,7 +155,6 @@ nbf:
  fp->_w = 0;
  }
  FUNLOCKFILE(fp);
- __atexit_register_cleanup(_cleanup);
 
  return (ret);
 }

enh
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remove unnecessary calls to __atexit_register_cleanup

enh
On Tue, Nov 18, 2014 at 11:02 PM, Philip Guenther <[hidden email]> wrote:

> On Tue, 18 Nov 2014, enh wrote:
>> On Tue, Nov 18, 2014 at 10:01 PM, Philip Guenther <[hidden email]> wrote:
> ...
>> > Maybe it would be better to do
>> >         if (!__sdidinit)
>> >                 __sinit();
>> >
>> > like other places?
>>
>> good point. i hadn't considered that stdin/stdout/stderr aren't
>> arrived at via __sfp.
>>
>> adding the __sinit stanza here would match the other places it currently
>> exists: places where buffers are filled/invalidated.
>
> This passes my test program above; oks?

i know i don't get a vote, but on Android this makes the common idiom
of fopen/fgets/fclose for a file in /proc about 2x faster. i'll commit
to Android anyway, and i'll let you know if we have any trouble with
it.

> Philip
>
>
> Index: makebuf.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdio/makebuf.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 makebuf.c
> --- makebuf.c   28 Dec 2005 18:50:22 -0000      1.8
> +++ makebuf.c   19 Nov 2014 07:00:53 -0000
> @@ -65,7 +65,6 @@ __smakebuf(FILE *fp)
>                 fp->_bf._size = 1;
>                 return;
>         }
> -       __atexit_register_cleanup(_cleanup);
>         flags |= __SMBF;
>         fp->_bf._base = fp->_p = p;
>         fp->_bf._size = size;
> Index: setvbuf.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdio/setvbuf.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 setvbuf.c
> --- setvbuf.c   9 Nov 2009 00:18:27 -0000       1.11
> +++ setvbuf.c   19 Nov 2014 07:00:53 -0000
> @@ -115,6 +115,13 @@ nbf:
>         }
>
>         /*
> +        * We're committed to buffering from here, so make sure we've
> +        * registered to flush buffers on exit.
> +        */
> +       if (!__sdidinit)
> +               __sinit();
> +
> +       /*
>          * Kill any seek optimization if the buffer is not the
>          * right size.
>          *
> @@ -148,7 +155,6 @@ nbf:
>                 fp->_w = 0;
>         }
>         FUNLOCKFILE(fp);
> -       __atexit_register_cleanup(_cleanup);
>
>         return (ret);
>  }

enh
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] remove unnecessary calls to __atexit_register_cleanup

enh
On Tue, Jan 13, 2015 at 2:23 PM, enh <[hidden email]> wrote:

> On Tue, Nov 18, 2014 at 11:02 PM, Philip Guenther <[hidden email]> wrote:
>> On Tue, 18 Nov 2014, enh wrote:
>>> On Tue, Nov 18, 2014 at 10:01 PM, Philip Guenther <[hidden email]> wrote:
>> ...
>>> > Maybe it would be better to do
>>> >         if (!__sdidinit)
>>> >                 __sinit();
>>> >
>>> > like other places?
>>>
>>> good point. i hadn't considered that stdin/stdout/stderr aren't
>>> arrived at via __sfp.
>>>
>>> adding the __sinit stanza here would match the other places it currently
>>> exists: places where buffers are filled/invalidated.
>>
>> This passes my test program above; oks?
>
> i know i don't get a vote, but on Android this makes the common idiom
> of fopen/fgets/fclose for a file in /proc about 2x faster. i'll commit
> to Android anyway, and i'll let you know if we have any trouble with
> it.

(it was pointed out to me off-list that this was submitted 15 hours
ago. i did check last week, and assumed nothing had changed since.
sorry for the noise!)

>> Philip
>>
>>
>> Index: makebuf.c
>> ===================================================================
>> RCS file: /cvs/src/lib/libc/stdio/makebuf.c,v
>> retrieving revision 1.8
>> diff -u -p -r1.8 makebuf.c
>> --- makebuf.c   28 Dec 2005 18:50:22 -0000      1.8
>> +++ makebuf.c   19 Nov 2014 07:00:53 -0000
>> @@ -65,7 +65,6 @@ __smakebuf(FILE *fp)
>>                 fp->_bf._size = 1;
>>                 return;
>>         }
>> -       __atexit_register_cleanup(_cleanup);
>>         flags |= __SMBF;
>>         fp->_bf._base = fp->_p = p;
>>         fp->_bf._size = size;
>> Index: setvbuf.c
>> ===================================================================
>> RCS file: /cvs/src/lib/libc/stdio/setvbuf.c,v
>> retrieving revision 1.11
>> diff -u -p -r1.11 setvbuf.c
>> --- setvbuf.c   9 Nov 2009 00:18:27 -0000       1.11
>> +++ setvbuf.c   19 Nov 2014 07:00:53 -0000
>> @@ -115,6 +115,13 @@ nbf:
>>         }
>>
>>         /*
>> +        * We're committed to buffering from here, so make sure we've
>> +        * registered to flush buffers on exit.
>> +        */
>> +       if (!__sdidinit)
>> +               __sinit();
>> +
>> +       /*
>>          * Kill any seek optimization if the buffer is not the
>>          * right size.
>>          *
>> @@ -148,7 +155,6 @@ nbf:
>>                 fp->_w = 0;
>>         }
>>         FUNLOCKFILE(fp);
>> -       __atexit_register_cleanup(_cleanup);
>>
>>         return (ret);
>>  }