benchtests: Improve readability of JSON output

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.

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

On 04/08/2014 11:28 AM, Will Newton wrote:

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

Since you posted this, I have to ask: Is there are concise way to 
implement this correctly, preserving the actual value of d in the output?

Getting this right is quite a challenge for other (real) JSON libraries. 
  They are also affected by the locale issue.

Will, many thanks for doing this!  I'll do a detailed review of this
later today or tomorrow if nobody else gets to it.

On Tue, Apr 08, 2014 at 11:36:17AM +0200, Florian Weimer wrote:
> On 04/08/2014 11:28 AM, Will Newton wrote:
> 
> >+void
> >+json_attr_double (json_ctx_t *ctx, const char *name, double d)
> >+{
> >+  if (!ctx->first_element)
> >+    {
> >+      fprintf (ctx->fp, ",\n");
> >+    }
> >+
> >+  ctx->first_element = false;
> >+
> >+  do_indent (ctx);
> >+
> >+  fprintf (ctx->fp, "\"%s\": %g", name, d);
> >+}
> 
> Since you posted this, I have to ask: Is there are concise way to implement
> this correctly, preserving the actual value of d in the output?

I guess the only way to preserve the exact value of d would be to
write out the actual memory contents.  In most cases this would be in
the IEEE754 double format.  So for a generic JSON library, they would
use uint64_t for double and actually do the conversion when they
serialize or de-serialize.

Siddhesh

On 8 April 2014 10:36, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/08/2014 11:28 AM, Will Newton wrote:
>
>> +void
>> +json_attr_double (json_ctx_t *ctx, const char *name, double d)
>> +{
>> +  if (!ctx->first_element)
>> +    {
>> +      fprintf (ctx->fp, ",\n");
>> +    }
>> +
>> +  ctx->first_element = false;
>> +
>> +  do_indent (ctx);
>> +
>> +  fprintf (ctx->fp, "\"%s\": %g", name, d);
>> +}
>
>
> Since you posted this, I have to ask: Is there are concise way to implement
> this correctly, preserving the actual value of d in the output?

Not to my knowledge. I guess it wouldn't hurt to increase the
precision a bit though. Either way there are going to be values we
cannot support such as NaN and Inf.

> Getting this right is quite a challenge for other (real) JSON libraries.
> They are also affected by the locale issue.

Luckily we don't need to be 100% general here and we have quite a
narrow use case. The code currently behaves exactly as it did before
the library code was extracted out so this patch does not actually
change behaviour. If we find we need multiple API functions to do
specific output formats for different purposes within the benchmarks
then I don't see that as a big problem.

On 04/08/2014 12:15 PM, Will Newton wrote:

>> Getting this right is quite a challenge for other (real) JSON libraries.
>> They are also affected by the locale issue.
>
> Luckily we don't need to be 100% general here and we have quite a
> narrow use case.

Sure, it's not a problem for benchtests reporting.  It's just something 
that keeps showing up in various C JSON libraries.

It's a pity that we don't have (on GNU/Linux at least) a canonical 
library to get gdtoa-type functionality from.

On 8 April 2014 11:24, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/08/2014 12:15 PM, Will Newton wrote:
>
>>> Getting this right is quite a challenge for other (real) JSON libraries.
>>> They are also affected by the locale issue.
>>
>>
>> Luckily we don't need to be 100% general here and we have quite a
>> narrow use case.
>
>
> Sure, it's not a problem for benchtests reporting.  It's just something that
> keeps showing up in various C JSON libraries.
>
> It's a pity that we don't have (on GNU/Linux at least) a canonical library
> to get gdtoa-type functionality from.

I think the "a" specifier would work but JSON does not support hex formats.

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
>

On 10 April 2014 17:30, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> 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 for the review, I have incorporated your suggestions. I have
also made some small API changes and additions so I will post a v2.

On Tue, 8 Apr 2014, Will Newton wrote:

> On 8 April 2014 11:24, Florian Weimer <fweimer@redhat.com> wrote:
> > On 04/08/2014 12:15 PM, Will Newton wrote:
> >
> >>> Getting this right is quite a challenge for other (real) JSON libraries.
> >>> They are also affected by the locale issue.
> >>
> >>
> >> Luckily we don't need to be 100% general here and we have quite a
> >> narrow use case.
> >
> >
> > Sure, it's not a problem for benchtests reporting.  It's just something that
> > keeps showing up in various C JSON libraries.
> >
> > It's a pity that we don't have (on GNU/Linux at least) a canonical library
> > to get gdtoa-type functionality from.
> 
> I think the "a" specifier would work but JSON does not support hex formats.

You can also use DECIMAL_DIG as the precision with "g" (presuming you are 
in round-to-nearest mode for both output and input) (or FLT_DECIMAL_DIG, 
DBL_DECIMAL_DIG, LDBL_DECIMAL_DIG, given C11).  Is there something making 
that unsuitable?

On 04/23/2014 04:26 PM, Joseph S. Myers wrote:

>>> It's a pity that we don't have (on GNU/Linux at least) a canonical library
>>> to get gdtoa-type functionality from.
>>
>> I think the "a" specifier would work but JSON does not support hex formats.
>
> You can also use DECIMAL_DIG as the precision with "g" (presuming you are
> in round-to-nearest mode for both output and input) (or FLT_DECIMAL_DIG,
> DBL_DECIMAL_DIG, LDBL_DECIMAL_DIG, given C11).  Is there something making
> that unsuitable?

What about the locale sensitivity?

On Thu, 24 Apr 2014, Florian Weimer wrote:

> On 04/23/2014 04:26 PM, Joseph S. Myers wrote:
> 
> > > > It's a pity that we don't have (on GNU/Linux at least) a canonical
> > > > library
> > > > to get gdtoa-type functionality from.
> > > 
> > > I think the "a" specifier would work but JSON does not support hex
> > > formats.
> > 
> > You can also use DECIMAL_DIG as the precision with "g" (presuming you are
> > in round-to-nearest mode for both output and input) (or FLT_DECIMAL_DIG,
> > DBL_DECIMAL_DIG, LDBL_DECIMAL_DIG, given C11).  Is there something making
> > that unsuitable?
> 
> What about the locale sensitivity?

Well, you can temporarily set the C locale for the current thread with 
uselocale, just as you can set and restore the rounding mode with 
fesetround.  Would it be better to have snprintf_l (see discussion Jun / 
Aug 2013; it appears Apple libc has such a function)?  I'm not sure if we 
have any clearly defined general principle for which (locale-dependent) 
functions have *_l variants and which require you to use uselocale if you 
want to call them with a locale other than the thread default.

Given snprintf_l *and* a GCC implementation of the TS 18661-1 FENV_ROUND 
pragma *and* glibc defining snprintf_l to be affected by constant rounding 
directions, this would reduce to a single function call in the scope of 
"#pragma STDC FENV_ROUND FE_TONEAREST" (with a previous computation of a 
locale_t value for the C locale to specify in that function call).

On 04/24/2014 12:38 PM, Joseph S. Myers wrote:

> Given snprintf_l *and* a GCC implementation of the TS 18661-1 FENV_ROUND
> pragma *and* glibc defining snprintf_l to be affected by constant rounding
> directions, this would reduce to a single function call in the scope of
> "#pragma STDC FENV_ROUND FE_TONEAREST" (with a previous computation of a
> locale_t value for the C locale to specify in that function call).

That would be fairly convenient.  People will still complain that 0.2 
comes out as 0.20000000000000001 (or whatever floating point values 
aren't printed in their shortest representation with DBL_DECIMAL_DIG), 
but at least the output is correct in a formal sense.