Re: PR30724, cygwin ld performance regression since 014a602b86

["Clément Chigot" <chigot@adacore.com>]

On Wed, Aug 9, 2023 at 1:21 AM Alan Modra <amodra@gmail.com> wrote:
>
> According to the reporter of this bug the newlib fseek implementation
> is likely slowed down by locking and fflush, only attempting to
> optimise seeks when the file is opened read-only.  Thus when writing
> the output we get a dramatic slowdown due to commit 014a602b86.
>
> Clément would you please check that this doesn't regress anything on
> mingw.

I'm not seeing any and the issue fixed by 014a602b86 is still fixed so
it looks good.
However, I have a question regarding the purpose of bfd_io_force.
Unless bfd_seek fails, it would be overridden right away. So are there
cases where bfd_seek fails and bfd doesn't report an error, continuing
to perform inputs/outputs as if nothing happens  ?


>         PR 30724
>         * bfd.c (enum bfd_last_io): New.
>         (struct bfd): Add last_io field.
>         * bfd-in2.h: Regenerate.
>         * bfd-io.c (bfd_bread, bfd_bwrite): Force seek if last_io is
>         opposite direction.
>         (bfd_seek): Reinstate optimisation for seek to same position.
>
> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index 5f49807d7f2..cc62ab19617 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -1916,6 +1916,14 @@ enum bfd_direction
>      both_direction = 3
>    };
>
> +enum bfd_last_io
> +  {
> +    bfd_io_seek = 0,
> +    bfd_io_read = 1,
> +    bfd_io_write = 2,
> +    bfd_io_force = 3
> +  };
> +
>  enum bfd_plugin_format
>    {
>      bfd_plugin_unknown = 0,
> @@ -2068,6 +2076,20 @@ struct bfd
>    /* The direction with which the BFD was opened.  */
>    ENUM_BITFIELD (bfd_direction) direction : 2;
>
> +  /* POSIX.1-2017 (IEEE Std 1003.1) says of fopen : "When a file is
> +     opened with update mode ('+' as the second or third character in
> +     the mode argument), both input and output may be performed on
> +     the associated stream.  However, the application shall ensure
> +     that output is not directly followed by input without an
> +     intervening call to fflush() or to a file positioning function
> +     (fseek(), fsetpos(), or rewind()), and input is not directly
> +     followed by output without an intervening call to a file
> +     positioning function, unless the input operation encounters
> +     end-of-file."
> +     This field tracks the last IO operation, so that bfd can insert
> +     a seek when IO direction changes.  */
> +  ENUM_BITFIELD (bfd_last_io) last_io : 2;
> +
>    /* Is the file descriptor being cached?  That is, can it be closed as
>       needed, and re-opened when accessed later?  */
>    unsigned int cacheable : 1;
> diff --git a/bfd/bfd.c b/bfd/bfd.c
> index e43a388ac72..88943a042d6 100644
> --- a/bfd/bfd.c
> +++ b/bfd/bfd.c
> @@ -53,6 +53,14 @@ EXTERNAL
>  .    both_direction = 3
>  .  };
>  .
> +.enum bfd_last_io
> +.  {
> +.    bfd_io_seek = 0,
> +.    bfd_io_read = 1,
> +.    bfd_io_write = 2,
> +.    bfd_io_force = 3
> +.  };
> +.
>  .enum bfd_plugin_format
>  .  {
>  .    bfd_plugin_unknown = 0,
> @@ -208,6 +216,20 @@ CODE_FRAGMENT
>  .  {* The direction with which the BFD was opened.  *}
>  .  ENUM_BITFIELD (bfd_direction) direction : 2;
>  .
> +.  {* POSIX.1-2017 (IEEE Std 1003.1) says of fopen : "When a file is
> +.     opened with update mode ('+' as the second or third character in
> +.     the mode argument), both input and output may be performed on
> +.     the associated stream.  However, the application shall ensure
> +.     that output is not directly followed by input without an
> +.     intervening call to fflush() or to a file positioning function
> +.     (fseek(), fsetpos(), or rewind()), and input is not directly
> +.     followed by output without an intervening call to a file
> +.     positioning function, unless the input operation encounters
> +.     end-of-file."
> +.     This field tracks the last IO operation, so that bfd can insert
> +.     a seek when IO direction changes.  *}
> +.  ENUM_BITFIELD (bfd_last_io) last_io : 2;
> +.
>  .  {* Is the file descriptor being cached?  That is, can it be closed as
>  .     needed, and re-opened when accessed later?  *}
>  .  unsigned int cacheable : 1;
> diff --git a/bfd/bfdio.c b/bfd/bfdio.c
> index 22c39a7b0cc..e0d47b3ee1c 100644
> --- a/bfd/bfdio.c
> +++ b/bfd/bfdio.c
> @@ -279,6 +279,14 @@ bfd_bread (void *ptr, bfd_size_type size, bfd *abfd)
>        return -1;
>      }
>
> +  if (abfd->last_io == bfd_io_write)
> +    {
> +      abfd->last_io = bfd_io_force;
> +      if (bfd_seek (abfd, 0, SEEK_CUR) != 0)
> +       return -1;
> +    }
> +  abfd->last_io = bfd_io_read;
> +
>    nread = abfd->iovec->bread (abfd, ptr, size);
>    if (nread != -1)
>      abfd->where += nread;
> @@ -313,6 +321,14 @@ bfd_bwrite (const void *ptr, bfd_size_type size, bfd *abfd)
>        return -1;
>      }
>
> +  if (abfd->last_io == bfd_io_read)
> +    {
> +      abfd->last_io = bfd_io_force;
> +      if (bfd_seek (abfd, 0, SEEK_CUR) != 0)
> +       return -1;
> +    }
> +  abfd->last_io = bfd_io_write;
> +
>    nwrote = abfd->iovec->bwrite (abfd, ptr, size);
>    if (nwrote != -1)
>      abfd->where += nwrote;
> @@ -456,6 +472,13 @@ bfd_seek (bfd *abfd, file_ptr position, int direction)
>    if (direction != SEEK_CUR)
>      position += offset;
>
> +  if (((direction == SEEK_CUR && position == 0)
> +       || (direction == SEEK_SET && (ufile_ptr) position == abfd->where))
> +      && abfd->last_io != bfd_io_force)
> +    return 0;
> +
> +  abfd->last_io = bfd_io_seek;
> +
>    result = abfd->iovec->bseek (abfd, position, direction);
>    if (result != 0)
>      {
>
> --
> Alan Modra
> Australia Development Lab, IBM