Received-SPF: pass (google.com: domain of linux-kernel+bounces-188119-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240519181716.4088459-1-irogers@google.com>
In-Reply-To: <20240519181716.4088459-1-irogers@google.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Thu, 23 May 2024 16:25:25 -0700
Message-ID: <CAM9d7chxsyZ+vnPXj0gc-mbADzwzQYi3qUwUohW-7He5KwMvHg@mail.gmail.com>
Subject: Re: [PATCH v1] tools api io: Move filling the io buffer to its own function
To: Ian Rogers <irogers@google.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>, linux-kernel@vger.kernel.org, 
	linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, May 19, 2024 at 11:17=E2=80=AFAM Ian Rogers <irogers@google.com> wr=
ote:
>
> In general a read fills 4kb so filling the buffer is a 1 in 4096
> operation, move it out of the io__get_char function to avoid some
> checking overhead and to better hint the function is good to inline.
>
> For perf's IO intensive internal (non-rigorous) benchmarks there's a
> near 8% improvement to kallsyms-parsing with a default build.

Oh, is it just from removing the io->eof check?  Otherwise I don't
see any difference.

Thanks,
Namhyung

>
> Before:
> ```
> $ perf bench internals all
> Computing performance of single threaded perf event synthesis by
> synthesizing events on the perf process itself:
>   Average synthesis took: 146.322 usec (+- 0.305 usec)
>   Average num. events: 61.000 (+- 0.000)
>   Average time per event 2.399 usec
>   Average data synthesis took: 145.056 usec (+- 0.155 usec)
>   Average num. events: 329.000 (+- 0.000)
>   Average time per event 0.441 usec
>
>   Average kallsyms__parse took: 162.313 ms (+- 0.599 ms)
> ...
> Computing performance of sysfs PMU event scan for 100 times
>   Average core PMU scanning took: 53.720 usec (+- 7.823 usec)
>   Average PMU scanning took: 375.145 usec (+- 23.974 usec)
> ```
> After:
> ```
> $ perf bench internals all
> Computing performance of single threaded perf event synthesis by
> synthesizing events on the perf process itself:
>   Average synthesis took: 127.829 usec (+- 0.079 usec)
>   Average num. events: 61.000 (+- 0.000)
>   Average time per event 2.096 usec
>   Average data synthesis took: 133.652 usec (+- 0.101 usec)
>   Average num. events: 327.000 (+- 0.000)
>   Average time per event 0.409 usec
>
>   Average kallsyms__parse took: 150.415 ms (+- 0.313 ms)
> ...
> Computing performance of sysfs PMU event scan for 100 times
>   Average core PMU scanning took: 47.790 usec (+- 1.178 usec)
>   Average PMU scanning took: 376.945 usec (+- 23.683 usec)
> ```
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/api/io.h | 69 +++++++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> index 84adf8102018..d3eb04d1bc89 100644
> --- a/tools/lib/api/io.h
> +++ b/tools/lib/api/io.h
> @@ -43,48 +43,55 @@ static inline void io__init(struct io *io, int fd,
>         io->eof =3D false;
>  }
>
> -/* Reads one character from the "io" file with similar semantics to fget=
c. */
> -static inline int io__get_char(struct io *io)
> +/* Read from fd filling the buffer. Called when io->data =3D=3D io->end.=
 */
> +static inline int io__fill_buffer(struct io *io)
>  {
> -       char *ptr =3D io->data;
> +       ssize_t n;
>
>         if (io->eof)
>                 return -1;
>
> -       if (ptr =3D=3D io->end) {
> -               ssize_t n;
> -
> -               if (io->timeout_ms !=3D 0) {
> -                       struct pollfd pfds[] =3D {
> -                               {
> -                                       .fd =3D io->fd,
> -                                       .events =3D POLLIN,
> -                               },
> -                       };
> -
> -                       n =3D poll(pfds, 1, io->timeout_ms);
> -                       if (n =3D=3D 0)
> -                               errno =3D ETIMEDOUT;
> -                       if (n > 0 && !(pfds[0].revents & POLLIN)) {
> -                               errno =3D EIO;
> -                               n =3D -1;
> -                       }
> -                       if (n <=3D 0) {
> -                               io->eof =3D true;
> -                               return -1;
> -                       }
> +       if (io->timeout_ms !=3D 0) {
> +               struct pollfd pfds[] =3D {
> +                       {
> +                               .fd =3D io->fd,
> +                               .events =3D POLLIN,
> +                       },
> +               };
> +
> +               n =3D poll(pfds, 1, io->timeout_ms);
> +               if (n =3D=3D 0)
> +                       errno =3D ETIMEDOUT;
> +               if (n > 0 && !(pfds[0].revents & POLLIN)) {
> +                       errno =3D EIO;
> +                       n =3D -1;
>                 }
> -               n =3D read(io->fd, io->buf, io->buf_len);
> -
>                 if (n <=3D 0) {
>                         io->eof =3D true;
>                         return -1;
>                 }
> -               ptr =3D &io->buf[0];
> -               io->end =3D &io->buf[n];
>         }
> -       io->data =3D ptr + 1;
> -       return *ptr;
> +       n =3D read(io->fd, io->buf, io->buf_len);
> +
> +       if (n <=3D 0) {
> +               io->eof =3D true;
> +               return -1;
> +       }
> +       io->data =3D &io->buf[0];
> +       io->end =3D &io->buf[n];
> +       return 0;
> +}
> +
> +/* Reads one character from the "io" file with similar semantics to fget=
c. */
> +static inline int io__get_char(struct io *io)
> +{
> +       if (io->data =3D=3D io->end) {
> +               int ret =3D io__fill_buffer(io);
> +
> +               if (ret)
> +                       return ret;
> +       }
> +       return *io->data++;
>  }
>
>  /* Read a hexadecimal value with no 0x prefix into the out argument hex.=
 If the
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>