diff mbox series

[v2,12/13] perf mmap: Lazily initialize zstd streams

Message ID 20231012062359.1616786-13-irogers@google.com (mailing list archive)
State Not Applicable
Headers show
Series Improvements to memory use | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Ian Rogers Oct. 12, 2023, 6:23 a.m. UTC 
Zstd streams create dictionaries that can require significant RAM,
especially when there is one per-CPU. Tools like perf record won't use
the streams without the -z option, and so the creation of the streams
is pure overhead. Switch to creating the streams on first use.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/compress.h |  1 +
 tools/perf/util/mmap.c     |  5 ++--
 tools/perf/util/mmap.h     |  1 -
 tools/perf/util/zstd.c     | 61 ++++++++++++++++++++------------------
 4 files changed, 35 insertions(+), 33 deletions(-)

Comments

Namhyung Kim Oct. 18, 2023, 11:21 p.m. UTC | #1 
On Wed, Oct 11, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote:
>
> Zstd streams create dictionaries that can require significant RAM,
> especially when there is one per-CPU. Tools like perf record won't use
> the streams without the -z option, and so the creation of the streams
> is pure overhead. Switch to creating the streams on first use.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/compress.h |  1 +
>  tools/perf/util/mmap.c     |  5 ++--
>  tools/perf/util/mmap.h     |  1 -
>  tools/perf/util/zstd.c     | 61 ++++++++++++++++++++------------------
>  4 files changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/tools/perf/util/compress.h b/tools/perf/util/compress.h
> index 0cd3369af2a4..9391850f1a7e 100644
> --- a/tools/perf/util/compress.h
> +++ b/tools/perf/util/compress.h
> @@ -21,6 +21,7 @@ struct zstd_data {
>  #ifdef HAVE_ZSTD_SUPPORT
>         ZSTD_CStream    *cstream;
>         ZSTD_DStream    *dstream;
> +       int comp_level;
>  #endif
>  };
>
> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> index 49093b21ee2d..122ee198a86e 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -295,15 +295,14 @@ int mmap__mmap(struct mmap *map, struct mmap_params *mp, int fd, struct perf_cpu
>
>         map->core.flush = mp->flush;
>
> -       map->comp_level = mp->comp_level;
>  #ifndef PYTHON_PERF
> -       if (zstd_init(&map->zstd_data, map->comp_level)) {
> +       if (zstd_init(&map->zstd_data, mp->comp_level)) {
>                 pr_debug2("failed to init mmap compressor, error %d\n", errno);
>                 return -1;
>         }
>  #endif
>
> -       if (map->comp_level && !perf_mmap__aio_enabled(map)) {
> +       if (mp->comp_level && !perf_mmap__aio_enabled(map)) {
>                 map->data = mmap(NULL, mmap__mmap_len(map), PROT_READ|PROT_WRITE,
>                                  MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
>                 if (map->data == MAP_FAILED) {
> diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
> index f944c3cd5efa..0df6e1621c7e 100644
> --- a/tools/perf/util/mmap.h
> +++ b/tools/perf/util/mmap.h
> @@ -39,7 +39,6 @@ struct mmap {
>  #endif
>         struct mmap_cpu_mask    affinity_mask;
>         void            *data;
> -       int             comp_level;
>         struct perf_data_file *file;
>         struct zstd_data      zstd_data;
>  };
> diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> index 48dd2b018c47..60f2d749b1c0 100644
> --- a/tools/perf/util/zstd.c
> +++ b/tools/perf/util/zstd.c
> @@ -7,35 +7,9 @@
>
>  int zstd_init(struct zstd_data *data, int level)
>  {
> -       size_t ret;
> -
> -       data->dstream = ZSTD_createDStream();
> -       if (data->dstream == NULL) {
> -               pr_err("Couldn't create decompression stream.\n");
> -               return -1;
> -       }
> -
> -       ret = ZSTD_initDStream(data->dstream);
> -       if (ZSTD_isError(ret)) {
> -               pr_err("Failed to initialize decompression stream: %s\n", ZSTD_getErrorName(ret));
> -               return -1;
> -       }
> -
> -       if (!level)
> -               return 0;
> -
> -       data->cstream = ZSTD_createCStream();
> -       if (data->cstream == NULL) {
> -               pr_err("Couldn't create compression stream.\n");
> -               return -1;
> -       }
> -
> -       ret = ZSTD_initCStream(data->cstream, level);
> -       if (ZSTD_isError(ret)) {
> -               pr_err("Failed to initialize compression stream: %s\n", ZSTD_getErrorName(ret));
> -               return -1;
> -       }
> -
> +       data->comp_level = level;
> +       data->dstream = NULL;
> +       data->cstream = NULL;
>         return 0;
>  }
>
> @@ -63,6 +37,21 @@ size_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t
>         ZSTD_outBuffer output;
>         void *record;
>
> +       if (!data->cstream) {
> +               data->cstream = ZSTD_createCStream();
> +               if (data->cstream == NULL) {
> +                       pr_err("Couldn't create compression stream.\n");
> +                       return -1;
> +               }
> +
> +               ret = ZSTD_initCStream(data->cstream, data->comp_level);
> +               if (ZSTD_isError(ret)) {
> +                       pr_err("Failed to initialize compression stream: %s\n",
> +                               ZSTD_getErrorName(ret));
> +                       return -1;

I'm not sure if the callers are ready to handle the failure.

Thanks,
Namhyung


> +               }
> +       }
> +
>         while (input.pos < input.size) {
>                 record = dst;
>                 size = process_header(record, 0);
> @@ -96,6 +85,20 @@ size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size
>         ZSTD_inBuffer input = { src, src_size, 0 };
>         ZSTD_outBuffer output = { dst, dst_size, 0 };
>
> +       if (!data->dstream) {
> +               data->dstream = ZSTD_createDStream();
> +               if (data->dstream == NULL) {
> +                       pr_err("Couldn't create decompression stream.\n");
> +                       return -1;
> +               }
> +
> +               ret = ZSTD_initDStream(data->dstream);
> +               if (ZSTD_isError(ret)) {
> +                       pr_err("Failed to initialize decompression stream: %s\n",
> +                               ZSTD_getErrorName(ret));
> +                       return -1;
> +               }
> +       }
>         while (input.pos < input.size) {
>                 ret = ZSTD_decompressStream(data->dstream, &output, &input);
>                 if (ZSTD_isError(ret)) {
> --
> 2.42.0.609.gbb76f46606-goog
>
Ian Rogers Oct. 24, 2023, 7:21 p.m. UTC | #2 
On Wed, Oct 18, 2023 at 4:21 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Oct 11, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Zstd streams create dictionaries that can require significant RAM,
> > especially when there is one per-CPU. Tools like perf record won't use
> > the streams without the -z option, and so the creation of the streams
> > is pure overhead. Switch to creating the streams on first use.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/compress.h |  1 +
> >  tools/perf/util/mmap.c     |  5 ++--
> >  tools/perf/util/mmap.h     |  1 -
> >  tools/perf/util/zstd.c     | 61 ++++++++++++++++++++------------------
> >  4 files changed, 35 insertions(+), 33 deletions(-)
> >
> > diff --git a/tools/perf/util/compress.h b/tools/perf/util/compress.h
> > index 0cd3369af2a4..9391850f1a7e 100644
> > --- a/tools/perf/util/compress.h
> > +++ b/tools/perf/util/compress.h
> > @@ -21,6 +21,7 @@ struct zstd_data {
> >  #ifdef HAVE_ZSTD_SUPPORT
> >         ZSTD_CStream    *cstream;
> >         ZSTD_DStream    *dstream;
> > +       int comp_level;
> >  #endif
> >  };
> >
> > diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> > index 49093b21ee2d..122ee198a86e 100644
> > --- a/tools/perf/util/mmap.c
> > +++ b/tools/perf/util/mmap.c
> > @@ -295,15 +295,14 @@ int mmap__mmap(struct mmap *map, struct mmap_params *mp, int fd, struct perf_cpu
> >
> >         map->core.flush = mp->flush;
> >
> > -       map->comp_level = mp->comp_level;
> >  #ifndef PYTHON_PERF
> > -       if (zstd_init(&map->zstd_data, map->comp_level)) {
> > +       if (zstd_init(&map->zstd_data, mp->comp_level)) {
> >                 pr_debug2("failed to init mmap compressor, error %d\n", errno);
> >                 return -1;
> >         }
> >  #endif
> >
> > -       if (map->comp_level && !perf_mmap__aio_enabled(map)) {
> > +       if (mp->comp_level && !perf_mmap__aio_enabled(map)) {
> >                 map->data = mmap(NULL, mmap__mmap_len(map), PROT_READ|PROT_WRITE,
> >                                  MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
> >                 if (map->data == MAP_FAILED) {
> > diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
> > index f944c3cd5efa..0df6e1621c7e 100644
> > --- a/tools/perf/util/mmap.h
> > +++ b/tools/perf/util/mmap.h
> > @@ -39,7 +39,6 @@ struct mmap {
> >  #endif
> >         struct mmap_cpu_mask    affinity_mask;
> >         void            *data;
> > -       int             comp_level;
> >         struct perf_data_file *file;
> >         struct zstd_data      zstd_data;
> >  };
> > diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> > index 48dd2b018c47..60f2d749b1c0 100644
> > --- a/tools/perf/util/zstd.c
> > +++ b/tools/perf/util/zstd.c
> > @@ -7,35 +7,9 @@
> >
> >  int zstd_init(struct zstd_data *data, int level)
> >  {
> > -       size_t ret;
> > -
> > -       data->dstream = ZSTD_createDStream();
> > -       if (data->dstream == NULL) {
> > -               pr_err("Couldn't create decompression stream.\n");
> > -               return -1;
> > -       }
> > -
> > -       ret = ZSTD_initDStream(data->dstream);
> > -       if (ZSTD_isError(ret)) {
> > -               pr_err("Failed to initialize decompression stream: %s\n", ZSTD_getErrorName(ret));
> > -               return -1;
> > -       }
> > -
> > -       if (!level)
> > -               return 0;
> > -
> > -       data->cstream = ZSTD_createCStream();
> > -       if (data->cstream == NULL) {
> > -               pr_err("Couldn't create compression stream.\n");
> > -               return -1;
> > -       }
> > -
> > -       ret = ZSTD_initCStream(data->cstream, level);
> > -       if (ZSTD_isError(ret)) {
> > -               pr_err("Failed to initialize compression stream: %s\n", ZSTD_getErrorName(ret));
> > -               return -1;
> > -       }
> > -
> > +       data->comp_level = level;
> > +       data->dstream = NULL;
> > +       data->cstream = NULL;
> >         return 0;
> >  }
> >
> > @@ -63,6 +37,21 @@ size_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t
> >         ZSTD_outBuffer output;
> >         void *record;
> >
> > +       if (!data->cstream) {
> > +               data->cstream = ZSTD_createCStream();
> > +               if (data->cstream == NULL) {
> > +                       pr_err("Couldn't create compression stream.\n");
> > +                       return -1;
> > +               }
> > +
> > +               ret = ZSTD_initCStream(data->cstream, data->comp_level);
> > +               if (ZSTD_isError(ret)) {
> > +                       pr_err("Failed to initialize compression stream: %s\n",
> > +                               ZSTD_getErrorName(ret));
> > +                       return -1;
>
> I'm not sure if the callers are ready to handle the failure.

Thanks, fixed in v3.

Ian

> Thanks,
> Namhyung
>
>
> > +               }
> > +       }
> > +
> >         while (input.pos < input.size) {
> >                 record = dst;
> >                 size = process_header(record, 0);
> > @@ -96,6 +85,20 @@ size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size
> >         ZSTD_inBuffer input = { src, src_size, 0 };
> >         ZSTD_outBuffer output = { dst, dst_size, 0 };
> >
> > +       if (!data->dstream) {
> > +               data->dstream = ZSTD_createDStream();
> > +               if (data->dstream == NULL) {
> > +                       pr_err("Couldn't create decompression stream.\n");
> > +                       return -1;
> > +               }
> > +
> > +               ret = ZSTD_initDStream(data->dstream);
> > +               if (ZSTD_isError(ret)) {
> > +                       pr_err("Failed to initialize decompression stream: %s\n",
> > +                               ZSTD_getErrorName(ret));
> > +                       return -1;
> > +               }
> > +       }
> >         while (input.pos < input.size) {
> >                 ret = ZSTD_decompressStream(data->dstream, &output, &input);
> >                 if (ZSTD_isError(ret)) {
> > --
> > 2.42.0.609.gbb76f46606-goog
> >
diff mbox series

Patch

diff --git a/tools/perf/util/compress.h b/tools/perf/util/compress.h
index 0cd3369af2a4..9391850f1a7e 100644
--- a/tools/perf/util/compress.h
+++ b/tools/perf/util/compress.h
@@ -21,6 +21,7 @@  struct zstd_data {
 #ifdef HAVE_ZSTD_SUPPORT
 	ZSTD_CStream	*cstream;
 	ZSTD_DStream	*dstream;
+	int comp_level;
 #endif
 };
 
diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
index 49093b21ee2d..122ee198a86e 100644
--- a/tools/perf/util/mmap.c
+++ b/tools/perf/util/mmap.c
@@ -295,15 +295,14 @@  int mmap__mmap(struct mmap *map, struct mmap_params *mp, int fd, struct perf_cpu
 
 	map->core.flush = mp->flush;
 
-	map->comp_level = mp->comp_level;
 #ifndef PYTHON_PERF
-	if (zstd_init(&map->zstd_data, map->comp_level)) {
+	if (zstd_init(&map->zstd_data, mp->comp_level)) {
 		pr_debug2("failed to init mmap compressor, error %d\n", errno);
 		return -1;
 	}
 #endif
 
-	if (map->comp_level && !perf_mmap__aio_enabled(map)) {
+	if (mp->comp_level && !perf_mmap__aio_enabled(map)) {
 		map->data = mmap(NULL, mmap__mmap_len(map), PROT_READ|PROT_WRITE,
 				 MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
 		if (map->data == MAP_FAILED) {
diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
index f944c3cd5efa..0df6e1621c7e 100644
--- a/tools/perf/util/mmap.h
+++ b/tools/perf/util/mmap.h
@@ -39,7 +39,6 @@  struct mmap {
 #endif
 	struct mmap_cpu_mask	affinity_mask;
 	void		*data;
-	int		comp_level;
 	struct perf_data_file *file;
 	struct zstd_data      zstd_data;
 };
diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
index 48dd2b018c47..60f2d749b1c0 100644
--- a/tools/perf/util/zstd.c
+++ b/tools/perf/util/zstd.c
@@ -7,35 +7,9 @@ 
 
 int zstd_init(struct zstd_data *data, int level)
 {
-	size_t ret;
-
-	data->dstream = ZSTD_createDStream();
-	if (data->dstream == NULL) {
-		pr_err("Couldn't create decompression stream.\n");
-		return -1;
-	}
-
-	ret = ZSTD_initDStream(data->dstream);
-	if (ZSTD_isError(ret)) {
-		pr_err("Failed to initialize decompression stream: %s\n", ZSTD_getErrorName(ret));
-		return -1;
-	}
-
-	if (!level)
-		return 0;
-
-	data->cstream = ZSTD_createCStream();
-	if (data->cstream == NULL) {
-		pr_err("Couldn't create compression stream.\n");
-		return -1;
-	}
-
-	ret = ZSTD_initCStream(data->cstream, level);
-	if (ZSTD_isError(ret)) {
-		pr_err("Failed to initialize compression stream: %s\n", ZSTD_getErrorName(ret));
-		return -1;
-	}
-
+	data->comp_level = level;
+	data->dstream = NULL;
+	data->cstream = NULL;
 	return 0;
 }
 
@@ -63,6 +37,21 @@  size_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t
 	ZSTD_outBuffer output;
 	void *record;
 
+	if (!data->cstream) {
+		data->cstream = ZSTD_createCStream();
+		if (data->cstream == NULL) {
+			pr_err("Couldn't create compression stream.\n");
+			return -1;
+		}
+
+		ret = ZSTD_initCStream(data->cstream, data->comp_level);
+		if (ZSTD_isError(ret)) {
+			pr_err("Failed to initialize compression stream: %s\n",
+				ZSTD_getErrorName(ret));
+			return -1;
+		}
+	}
+
 	while (input.pos < input.size) {
 		record = dst;
 		size = process_header(record, 0);
@@ -96,6 +85,20 @@  size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size
 	ZSTD_inBuffer input = { src, src_size, 0 };
 	ZSTD_outBuffer output = { dst, dst_size, 0 };
 
+	if (!data->dstream) {
+		data->dstream = ZSTD_createDStream();
+		if (data->dstream == NULL) {
+			pr_err("Couldn't create decompression stream.\n");
+			return -1;
+		}
+
+		ret = ZSTD_initDStream(data->dstream);
+		if (ZSTD_isError(ret)) {
+			pr_err("Failed to initialize decompression stream: %s\n",
+				ZSTD_getErrorName(ret));
+			return -1;
+		}
+	}
 	while (input.pos < input.size) {
 		ret = ZSTD_decompressStream(data->dstream, &output, &input);
 		if (ZSTD_isError(ret)) {