Patchwork Optimize screendump

When running kvm-autotest, fputc() is often the second highest (sometimes #1)
function showing up in a profile.  This is due to fputc() locking the file
for every byte written.

Optimize by using fputc_unlocked().  Since the file is local to the caller,
clearly no locking is needed.  According to the manual, _GNU_SOURCE is all
that's needed for the function to be present.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/vga.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

On Sun, 19 Jun 2011, Avi Kivity wrote:

CONFORMING TO
       The     four	functions    getc_unlocked(), getchar_unlocked(),
       putc_unlocked(), putchar_unlocked() are in POSIX.1-2001.
       The non-standard *_unlocked() variants occur on a few Unix systems, and
       are available in recent glibc.  They should probably not be used.

> When running kvm-autotest, fputc() is often the second highest (sometimes #1)
> function showing up in a profile.  This is due to fputc() locking the file
> for every byte written.
> 
> Optimize by using fputc_unlocked().  Since the file is local to the caller,
> clearly no locking is needed.  According to the manual, _GNU_SOURCE is all
> that's needed for the function to be present.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  hw/vga.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vga.c b/hw/vga.c
> index d5bc582..8b63358 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2369,9 +2369,9 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>                  (ds->pf.gmax + 1);
>              b = ((v >> ds->pf.bshift) & ds->pf.bmax) * 256 /
>                  (ds->pf.bmax + 1);
> -            fputc(r, f);
> -            fputc(g, f);
> -            fputc(b, f);
> +            fputc_unlocked(r, f);
> +            fputc_unlocked(g, f);
> +            fputc_unlocked(b, f);
>              d += ds->pf.bytes_per_pixel;
>          }
>          d1 += ds->linesize;
>

On Sun, Jun 19, 2011 at 3:54 PM, Avi Kivity <avi@redhat.com> wrote:
> When running kvm-autotest, fputc() is often the second highest (sometimes #1)
> function showing up in a profile.  This is due to fputc() locking the file
> for every byte written.
>
> Optimize by using fputc_unlocked().  Since the file is local to the caller,
> clearly no locking is needed.  According to the manual, _GNU_SOURCE is all
> that's needed for the function to be present.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  hw/vga.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

"The nonstandard *_unlocked() variants occur on a few Unix systems,
and are available in recent glibc. They should probably not be used."

I wonder if this will break non-Linux platforms.  Perhaps buffer an
entire row of pixels instead and only fwrite(3) at the end of the
outer loop.

Stefan

On 06/19/2011 06:22 PM, Stefan Hajnoczi wrote:
> I wonder if this will break non-Linux platforms.  Perhaps buffer an
> entire row of pixels instead and only fwrite(3) at the end of the
> outer loop.

That's how I wrote this in the first place.  Since the consensus is 
against these functions, I'll submit that version instead.

Am 19.06.2011 um 17:46 schrieb Avi Kivity:

> On 06/19/2011 06:22 PM, Stefan Hajnoczi wrote:
>> I wonder if this will break non-Linux platforms.  Perhaps buffer an
>> entire row of pixels instead and only fwrite(3) at the end of the
>> outer loop.
>
> That's how I wrote this in the first place.  Since the consensus is  
> against these functions, I'll submit that version instead.

Maybe add a qemu_fputc_unlocked() and do a configure check for it?

Andreas

On 06/19/2011 06:53 PM, Andreas Färber wrote:
> Am 19.06.2011 um 17:46 schrieb Avi Kivity:
>
>> On 06/19/2011 06:22 PM, Stefan Hajnoczi wrote:
>>> I wonder if this will break non-Linux platforms.  Perhaps buffer an
>>> entire row of pixels instead and only fwrite(3) at the end of the
>>> outer loop.
>>
>> That's how I wrote this in the first place.  Since the consensus is 
>> against these functions, I'll submit that version instead.
>
> Maybe add a qemu_fputc_unlocked() and do a configure check for it?

Good idea.  I'll try that, unless people disagree.

On 19.06.2011, at 18:04, Avi Kivity wrote:

> On 06/19/2011 06:53 PM, Andreas Färber wrote:
>> Am 19.06.2011 um 17:46 schrieb Avi Kivity:
>> 
>>> On 06/19/2011 06:22 PM, Stefan Hajnoczi wrote:
>>>> I wonder if this will break non-Linux platforms.  Perhaps buffer an
>>>> entire row of pixels instead and only fwrite(3) at the end of the
>>>> outer loop.
>>> 
>>> That's how I wrote this in the first place.  Since the consensus is against these functions, I'll submit that version instead.
>> 
>> Maybe add a qemu_fputc_unlocked() and do a configure check for it?
> 
> Good idea.  I'll try that, unless people disagree.

Writing by row should be faster and pretty straight forward, no?


Alex

On 06/19/2011 08:00 PM, Alexander Graf wrote:
> On 19.06.2011, at 18:04, Avi Kivity wrote:
>
> >  On 06/19/2011 06:53 PM, Andreas Färber wrote:
> >>  Am 19.06.2011 um 17:46 schrieb Avi Kivity:
> >>
> >>>  On 06/19/2011 06:22 PM, Stefan Hajnoczi wrote:
> >>>>  I wonder if this will break non-Linux platforms.  Perhaps buffer an
> >>>>  entire row of pixels instead and only fwrite(3) at the end of the
> >>>>  outer loop.
> >>>
> >>>  That's how I wrote this in the first place.  Since the consensus is against these functions, I'll submit that version instead.
> >>
> >>  Maybe add a qemu_fputc_unlocked() and do a configure check for it?
> >
> >  Good idea.  I'll try that, unless people disagree.
>
> Writing by row should be faster and pretty straight forward, no?

I don't see how it's faster, but I guess I'll do that, it's a local 
issue and is best addressed locally.