Re: [PATCH] benchtests: Improve readability of JSON output

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Tue, Apr 08, 2014 at 10:28:10AM +0100, Will Newton wrote:
> Add a small library to print JSON values and use it to improve the
> readability of the benchmark output and the readability of the
> benchmark code.

Sorry about the delay, but here's the review.  The patch looks good to
me barring a few nits that you can fix before committing.

Thanks,
Siddhesh

> 
> ChangeLog:
> 
> 2014-04-08  Will Newton  <will.newton@linaro.org>
> 
> 	* benchtests/Makefile (extra-objs): Add json-lib.o.
> 	(bench-func): Tidy up JSON output.
> 	* benchtests/bench-skeleton.c: Include json-lib.h.
> 	(main): Use JSON library functions to do output of
> 	benchmark results.
> 	* benchtests/bench-timing-type.c (main): Output the
> 	timing type simply, leaving formatting to the user.
> 	* benchtests/json-lib.c: New file.
> 	* benchtests/json-lib.h: Likewise.
> ---
>  benchtests/Makefile            |  13 ++--
>  benchtests/bench-skeleton.c    |  39 ++++++------
>  benchtests/bench-timing-type.c |   2 +-
>  benchtests/json-lib.c          | 132 +++++++++++++++++++++++++++++++++++++++++
>  benchtests/json-lib.h          |  42 +++++++++++++
>  5 files changed, 205 insertions(+), 23 deletions(-)
>  create mode 100644 benchtests/json-lib.c
>  create mode 100644 benchtests/json-lib.h
> 
> diff --git a/benchtests/Makefile b/benchtests/Makefile
> index ca635cf..a788082 100644
> --- a/benchtests/Makefile
> +++ b/benchtests/Makefile
> @@ -100,6 +100,8 @@ cpp-srcs-left := $(binaries-benchset:=.c) $(binaries-bench:=.c)
>  lib := nonlib
>  include $(patsubst %,$(..)cppflags-iterator.mk,$(cpp-srcs-left))
>  
> +extra-objs += json-lib.o
> +
>  bench-deps := bench-skeleton.c bench-timing.h Makefile
>  
>  run-bench = $(test-wrapper-env) \
> @@ -126,9 +128,9 @@ bench-set: $(binaries-benchset)
>  # so one could even execute them individually and process it using any JSON
>  # capable language or tool.
>  bench-func: $(binaries-bench)
> -	{ echo "{"; \
> -	$(timing-type); \
> -	echo "  ,\"functions\": {"; \
> +	{ timing_type=$$($(timing-type)); \
> +	echo "{\"timing_type\": \"$${timing_type}\","; \
> +	echo " \"functions\": {"; \
>  	for run in $^; do \
>  	  if ! [ "x$${run}" = "x$<" ]; then \
>  	    echo ","; \
> @@ -136,14 +138,15 @@ bench-func: $(binaries-bench)
>  	  echo "Running $${run}" >&2; \
>  	  $(run-bench) $(DETAILED_OPT); \
>  	done; \
> -	echo "  }"; \
> +	echo; \
> +	echo " }"; \
>  	echo "}"; } > $(objpfx)bench.out-tmp; \
>  	if [ -f $(objpfx)bench.out ]; then \
>  	  mv -f $(objpfx)bench.out $(objpfx)bench.out.old; \
>  	fi; \
>  	mv -f $(objpfx)bench.out-tmp $(objpfx)bench.out
>  
> -$(timing-type) $(binaries-bench) $(binaries-benchset): %: %.o \
> +$(timing-type) $(binaries-bench) $(binaries-benchset): %: %.o $(objpfx)json-lib.o \
>    $(sort $(filter $(common-objpfx)lib%,$(link-libc))) \
>    $(addprefix $(csu-objpfx),start.o) $(+preinit) $(+postinit)
>  	$(+link)
> diff --git a/benchtests/bench-skeleton.c b/benchtests/bench-skeleton.c
> index 0c7d744..ee37173 100644
> --- a/benchtests/bench-skeleton.c
> +++ b/benchtests/bench-skeleton.c
> @@ -23,6 +23,7 @@
>  #include <time.h>
>  #include <inttypes.h>
>  #include "bench-timing.h"
> +#include "json-lib.h"
>  
>  volatile unsigned int dontoptimize = 0;
>  
> @@ -50,6 +51,7 @@ main (int argc, char **argv)
>    struct timespec runtime;
>    timing_t start, end;
>    bool detailed = false;
> +  json_ctx_t json_ctx;
>  
>    if (argc == 2 && !strcmp (argv[1], "-d"))
>      detailed = true;
> @@ -64,13 +66,13 @@ main (int argc, char **argv)
>  
>    iters = 1000 * res;
>  
> +  json_init (&json_ctx, 2, stdout);
> +
>    /* Begin function.  */
> -  printf ("\"%s\": {\n", FUNCNAME);
> +  json_object_begin (&json_ctx, FUNCNAME);
>  
>    for (int v = 0; v < NUM_VARIANTS; v++)
>      {
> -      if (v)
> -	putc (',', stdout);
>        /* Run for approximately DURATION seconds.  */
>        clock_gettime (CLOCK_MONOTONIC_RAW, &runtime);
>        runtime.tv_sec += DURATION;
> @@ -119,28 +121,31 @@ main (int argc, char **argv)
>        d_total_s = total;
>        d_iters = iters;
>  
> -      printf ("\"%s\": {\n", VARIANT (v));
> -      printf ("\"duration\": %g, \"iterations\": %g, "
> -	      "\"max\": %g, \"min\": %g, \"mean\": %g\n",
> -	      d_total_s, d_total_i, max / d_iters, min / d_iters,
> -	      d_total_s / d_total_i);
> +      /* Begin variant.  */
> +      json_object_begin (&json_ctx, VARIANT (v));
> +
> +      json_attr_double (&json_ctx, "duration", d_total_s);
> +      json_attr_double (&json_ctx, "iterations", d_total_i);
> +      json_attr_double (&json_ctx, "max", max / d_iters);
> +      json_attr_double (&json_ctx, "min", min / d_iters);
> +      json_attr_double (&json_ctx, "mean", d_total_s / d_total_i);
>  
>        if (detailed)
>  	{
> -	  printf (",\n\"timings\": [");
> +	  json_array_begin (&json_ctx, "timings");
> +
>  	  for (int i = 0; i < NUM_SAMPLES (v); i++)
> -	    {
> -	      if (i > 0)
> -		putc (',', stdout);
> -	      printf ("%g", RESULT (v, i));
> -	    }
> -	  puts ("]");
> +	    json_element_double (&json_ctx, RESULT (v, i));
> +
> +	  json_array_end (&json_ctx);
>  	}
> -      puts ("}");
> +
> +      /* End variant.  */
> +      json_object_end (&json_ctx);
>      }
>  
>    /* End function.  */
> -  puts ("}");
> +  json_object_end (&json_ctx);
>  
>    return 0;
>  }
> diff --git a/benchtests/bench-timing-type.c b/benchtests/bench-timing-type.c
> index 903a61f..64219ff 100644
> --- a/benchtests/bench-timing-type.c
> +++ b/benchtests/bench-timing-type.c
> @@ -22,6 +22,6 @@
>  int
>  main (int argc, char **argv)
>  {
> -  printf ("\"timing-type\": \"%s\"\n", TIMING_TYPE);
> +  puts (TIMING_TYPE);
>    return 0;
>  }
> diff --git a/benchtests/json-lib.c b/benchtests/json-lib.c
> new file mode 100644
> index 0000000..87f21ae
> --- /dev/null
> +++ b/benchtests/json-lib.c
> @@ -0,0 +1,132 @@
> +/* Simple library for printing JSON data.
> +   Copyright (C) 2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <string.h>
> +
> +#include "json-lib.h"
> +
> +void
> +json_init (json_ctx_t *ctx, unsigned int indent_level, FILE *fp)
> +{
> +  ctx->indent_level = indent_level;
> +  ctx->fp = fp;
> +  ctx->first_element = true;
> +}
> +
> +static void
> +do_indent (json_ctx_t *ctx)
> +{
> +  char indent_buf[ctx->indent_level + 1];
> +
> +  memset (indent_buf, ' ', ctx->indent_level + 1);
> +  indent_buf[ctx->indent_level] = '\0';
> +
> +  fputs (indent_buf, ctx->fp);
> +}
> +
> +void
> +json_object_begin (json_ctx_t *ctx, const char *name)
> +{
> +  if (!ctx->first_element)
> +    fprintf (ctx->fp, ",\n");
> +
> +  do_indent (ctx);
> +
> +  fprintf (ctx->fp, "\"%s\": {\n", name);
> +
> +  ctx->indent_level++;
> +  ctx->first_element = true;
> +}
> +
> +void
> +json_object_end (json_ctx_t *ctx)
> +{
> +  ctx->indent_level--;
> +  ctx->first_element = false;
> +
> +  fputs ("\n", ctx->fp);
> +
> +  do_indent (ctx);
> +
> +  fputs ("}", ctx->fp);
> +}
> +
> +void
> +json_array_begin (json_ctx_t *ctx, const char *name)
> +{
> +  if (!ctx->first_element)
> +    fprintf (ctx->fp, ",\n");
> +
> +  do_indent (ctx);
> +
> +  fprintf (ctx->fp, "\"%s\": [", name);
> +
> +  ctx->indent_level++;
> +  ctx->first_element = true;
> +}
> +
> +void
> +json_element_double (json_ctx_t *ctx, double d)
> +{
> +  if (!ctx->first_element)
> +    fprintf (ctx->fp, ", %g", d);
> +  else
> +    {
> +      fprintf (ctx->fp, "%g", d);
> +      ctx->first_element = false;
> +    }
> +}
> +
> +void
> +json_array_end (json_ctx_t *ctx)
> +{
> +  ctx->indent_level--;
> +  ctx->first_element = false;
> +
> +  fputs ("]", ctx->fp);
> +}
> +
> +void
> +json_attr_string (json_ctx_t *ctx, const char *name, const char *s)
> +{
> +  if (!ctx->first_element)
> +    {
> +      fprintf (ctx->fp, ",\n");
> +    }

Redundant braces.

> +
> +  ctx->first_element = false;

It would be nicer to put this in an else block to get rid of a
redundant assignment.  I guess the compiler must be doing that anyway
though.  I don't have a very strong opinion on this though, since it
is not exactly performance sensitive.

> +
> +  do_indent (ctx);
> +
> +  fprintf (ctx->fp, "\"%s\": \"%s\"", name, s);
> +}
> +
> +void
> +json_attr_double (json_ctx_t *ctx, const char *name, double d)
> +{
> +  if (!ctx->first_element)
> +    {
> +      fprintf (ctx->fp, ",\n");
> +    }
> +

Redundant braces.

> +  ctx->first_element = false;
> +

Maybe put in else block as suggested above.

> +  do_indent (ctx);
> +
> +  fprintf (ctx->fp, "\"%s\": %g", name, d);
> +}
> diff --git a/benchtests/json-lib.h b/benchtests/json-lib.h
> new file mode 100644
> index 0000000..3cbbab3
> --- /dev/null
> +++ b/benchtests/json-lib.h
> @@ -0,0 +1,42 @@
> +/* Simple library for printing JSON data.
> +   Copyright (C) 2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef __JSON_LIB_H__
> +#define __JSON_LIB_H__
> +
> +#include <stdbool.h>
> +#include <stdio.h>
> +
> +struct json_ctx {

The brace should be on the next line.

> +  FILE *fp;
> +  unsigned int indent_level;
> +  bool first_element;
> +};
> +
> +typedef struct json_ctx json_ctx_t;
> +
> +void json_init (json_ctx_t *ctx, unsigned int indent_level, FILE *fp);
> +void json_object_begin (json_ctx_t *ctx, const char *name);
> +void json_object_end (json_ctx_t *ctx);
> +void json_array_begin (json_ctx_t *ctx, const char *name);
> +void json_element_double (json_ctx_t *ctx, double d);
> +void json_array_end (json_ctx_t *ctx);
> +void json_attr_string (json_ctx_t *ctx, const char *name, const char *s);
> +void json_attr_double (json_ctx_t *ctx, const char *name, double d);
> +
> +#endif
> -- 
> 1.8.1.4
>

Attachment: pgpEecrRjzSI5.pgp
Description: PGP signature