diff mbox series

[v3,4/5] benchtests: Fix validate_benchout.py exceptions

Message ID 20210805075144.433644-1-naohirot@fujitsu.com
State Superseded
Headers
Series benchtests: Add memset zero fill benchmark test |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Naohiro Tamura Aug. 5, 2021, 7:51 a.m. UTC 
  This patch fixed validate_benchout.py two exceptions, AttributeError
if benchout_strings.schema.json is specified and
json.decoder.JSONDecodeError if benchout is not JSON.
---
 benchtests/scripts/import_bench.py      | 5 ++++-
 benchtests/scripts/validate_benchout.py | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

develop--- via Libc-alpha Sept. 8, 2021, 1:55 a.m. UTC | #1 
Hi all, is there any comment?
https://sourceware.org/pipermail/libc-alpha/2021-August/129841.html
Thanks.
Naohiro
> -----Original Message-----
> From: Naohiro Tamura <naohirot@fujitsu.com>
> Sent: Thursday, August 5, 2021 4:52 PM
> To: libc-alpha@sourceware.org
> Cc: Tamura, Naohiro/田村 直広 <naohirot@fujitsu.com>
> Subject: [PATCH v3 4/5] benchtests: Fix validate_benchout.py exceptions
> 
> This patch fixed validate_benchout.py two exceptions, AttributeError
> if benchout_strings.schema.json is specified and
> json.decoder.JSONDecodeError if benchout is not JSON.
> ---
>  benchtests/scripts/import_bench.py      | 5 ++++-
>  benchtests/scripts/validate_benchout.py | 6 +++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/benchtests/scripts/import_bench.py b/benchtests/scripts/import_bench.py
> index a799b4e1b7dc..e3337ca5d638 100644
> --- a/benchtests/scripts/import_bench.py
> +++ b/benchtests/scripts/import_bench.py
> @@ -104,7 +104,10 @@ def do_for_all_timings(bench, callback):
>      """
>      for func in bench['functions'].keys():
>          for k in bench['functions'][func].keys():
> -            if 'timings' not in bench['functions'][func][k].keys():
> +            try:
> +                if 'timings' not in bench['functions'][func][k].keys():
> +                    continue
> +            except AttributeError:
>                  continue
> 
>              callback(bench, func, k)
> diff --git a/benchtests/scripts/validate_benchout.py b/benchtests/scripts/validate_benchout.py
> index 47df33ed0252..00d5fa0ee5eb 100755
> --- a/benchtests/scripts/validate_benchout.py
> +++ b/benchtests/scripts/validate_benchout.py
> @@ -73,11 +73,15 @@ def main(args):
> 
>      except bench.validator.ValidationError as e:
>          return print_and_exit("Invalid benchmark output: %s" % e.message,
> -            os.EX_DATAERR)
> +                os.EX_DATAERR)
> 
>      except bench.validator.SchemaError as e:
>          return print_and_exit("Invalid schema: %s" % e.message, os.EX_DATAERR)
> 
> +    except json.decoder.JSONDecodeError as e:
> +        return print_and_exit("Benchmark output in %s is not JSON." % args[0],
> +                os.EX_DATAERR)
> +
>      print("Benchmark output in %s is valid." % args[0])
>      return os.EX_OK
> 
> --
> 2.17.1
Siddhesh Poyarekar Sept. 13, 2021, 3:42 a.m. UTC | #2 
On 8/5/21 1:21 PM, Naohiro Tamura via Libc-alpha wrote:
> This patch fixed validate_benchout.py two exceptions, AttributeError
> if benchout_strings.schema.json is specified and
> json.decoder.JSONDecodeError if benchout is not JSON.
> ---
>   benchtests/scripts/import_bench.py      | 5 ++++-
>   benchtests/scripts/validate_benchout.py | 6 +++++-
>   2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/benchtests/scripts/import_bench.py b/benchtests/scripts/import_bench.py
> index a799b4e1b7dc..e3337ca5d638 100644
> --- a/benchtests/scripts/import_bench.py
> +++ b/benchtests/scripts/import_bench.py
> @@ -104,7 +104,10 @@ def do_for_all_timings(bench, callback):
>       """
>       for func in bench['functions'].keys():
>           for k in bench['functions'][func].keys():
> -            if 'timings' not in bench['functions'][func][k].keys():
> +            try:
> +                if 'timings' not in bench['functions'][func][k].keys():
> +                    continue
> +            except AttributeError:
>                   continue

When do you get an AttributeError here?

>   
>               callback(bench, func, k)
> diff --git a/benchtests/scripts/validate_benchout.py b/benchtests/scripts/validate_benchout.py
> index 47df33ed0252..00d5fa0ee5eb 100755
> --- a/benchtests/scripts/validate_benchout.py
> +++ b/benchtests/scripts/validate_benchout.py
> @@ -73,11 +73,15 @@ def main(args):
>   
>       except bench.validator.ValidationError as e:
>           return print_and_exit("Invalid benchmark output: %s" % e.message,
> -            os.EX_DATAERR)
> +                os.EX_DATAERR)
>   
>       except bench.validator.SchemaError as e:
>           return print_and_exit("Invalid schema: %s" % e.message, os.EX_DATAERR)
>   
> +    except json.decoder.JSONDecodeError as e:
> +        return print_and_exit("Benchmark output in %s is not JSON." % args[0],
> +                os.EX_DATAERR)
> +
>       print("Benchmark output in %s is valid." % args[0])
>       return os.EX_OK
>   
>
Siddhesh Poyarekar Sept. 13, 2021, 3:50 a.m. UTC | #3 
On 9/13/21 9:12 AM, Siddhesh Poyarekar wrote:
>> --- a/benchtests/scripts/import_bench.py
>> +++ b/benchtests/scripts/import_bench.py
>> @@ -104,7 +104,10 @@ def do_for_all_timings(bench, callback):
>>       """
>>       for func in bench['functions'].keys():
>>           for k in bench['functions'][func].keys():
>> -            if 'timings' not in bench['functions'][func][k].keys():
>> +            try:
>> +                if 'timings' not in bench['functions'][func][k].keys():
>> +                    continue
>> +            except AttributeError:
>>                   continue
> 
> When do you get an AttributeError here?
> 

OK the one possibility I can think of is when 
bench['functions'][func][k] is None.  This implies the existence of a 
benchmark output that has a function variant without any inputs and 
hence, without any benchmark data.  That should be invalid, in which 
case the benchmark should be fixed, not the validator.

Thanks,
Siddhesh
develop--- via Libc-alpha Sept. 13, 2021, 1:46 p.m. UTC | #4 
Hi Siddhesh,

Thank you for the two merges and this review.

> From: Siddhesh Poyarekar <siddhesh@gotplt.org>
> Sent: Monday, 13 September 2021 12:50
> On 9/13/21 9:12 AM, Siddhesh Poyarekar wrote:
> >> --- a/benchtests/scripts/import_bench.py
> >> +++ b/benchtests/scripts/import_bench.py
> >> @@ -104,7 +104,10 @@ def do_for_all_timings(bench, callback):
> >>       """
> >>       for func in bench['functions'].keys():
> >>           for k in bench['functions'][func].keys():
> >> -            if 'timings' not in bench['functions'][func][k].keys():
> >> +            try:
> >> +                if 'timings' not in bench['functions'][func][k].keys():
> >> +                    continue
> >> +            except AttributeError:
> >>                   continue
> >
> > When do you get an AttributeError here?
> >
> 
> OK the one possibility I can think of is when
> bench['functions'][func][k] is None.  This implies the existence of a
> benchmark output that has a function variant without any inputs and
> hence, without any benchmark data.  That should be invalid, in which
> case the benchmark should be fixed, not the validator.

AttributeError unconditionally occurs with a correct JSON benchout
file such as below because the code
"bench['functions'][func][k].keys()" is either  "bench-variant",
"ifunc", or "results" that doesn't have keys()."

$ ~/glibc/benchtests/scripts/validate_benchout.py bench-memcpy.out \
  ~/glibc/benchtests/scripts/benchout_strings.schema.json
Traceback (most recent call last):
  File "/home/naohirot/work/github/glibc/benchtests/scripts/validate_benchout.py", line 86, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/naohirot/work/github/glibc/benchtests/scripts/validate_benchout.py", line 69, in main
    bench.parse_bench(args[0], args[1])
  File "/home/naohirot/work/github/glibc/benchtests/scripts/import_bench.py", line 139, in parse_bench
    do_for_all_timings(bench, lambda b, f, v:
  File "/home/naohirot/work/github/glibc/benchtests/scripts/import_bench.py", line 107, in do_for_all_timings
    if 'timings' not in bench['functions'][func][k].keys():
AttributeError: 'str' object has no attribute 'keys'

$ cat bench-memcpy.out
  1 {
  2  "timing_type": "hp_timing",
  3  "functions": {
  4   "memcpy": {
  5    "bench-variant": "default",
  6    "ifuncs": ["generic_memcpy", "__memcpy_thunderx", "__memcpy_thunderx2", "__memcpy_falkor", "__memcpy_simd", "__memcpy_a64fx", "__memcpy_generic"],
  7    "results": [
  8     {
  9      "length": 1,
 10      "align1": 0,
 11      "align2": 0,
 12      "dst > src": 0,
 13      "timings": [10.9326, 11.0449, 11.5515, 13.5693, 11.5198, 6.77368, 11.5259]
 14     },
 ...
 
I found out that the implementation is not right, and sent V4 patch [1].
Please find it.

[1] https://sourceware.org/pipermail/libc-alpha/2021-September/130915.html

Thanks.
Naohiro
diff mbox series

Patch

diff --git a/benchtests/scripts/import_bench.py b/benchtests/scripts/import_bench.py
index a799b4e1b7dc..e3337ca5d638 100644
--- a/benchtests/scripts/import_bench.py
+++ b/benchtests/scripts/import_bench.py
@@ -104,7 +104,10 @@  def do_for_all_timings(bench, callback):
     """
     for func in bench['functions'].keys():
         for k in bench['functions'][func].keys():
-            if 'timings' not in bench['functions'][func][k].keys():
+            try:
+                if 'timings' not in bench['functions'][func][k].keys():
+                    continue
+            except AttributeError:
                 continue
 
             callback(bench, func, k)
diff --git a/benchtests/scripts/validate_benchout.py b/benchtests/scripts/validate_benchout.py
index 47df33ed0252..00d5fa0ee5eb 100755
--- a/benchtests/scripts/validate_benchout.py
+++ b/benchtests/scripts/validate_benchout.py
@@ -73,11 +73,15 @@  def main(args):
 
     except bench.validator.ValidationError as e:
         return print_and_exit("Invalid benchmark output: %s" % e.message,
-            os.EX_DATAERR)
+                os.EX_DATAERR)
 
     except bench.validator.SchemaError as e:
         return print_and_exit("Invalid schema: %s" % e.message, os.EX_DATAERR)
 
+    except json.decoder.JSONDecodeError as e:
+        return print_and_exit("Benchmark output in %s is not JSON." % args[0],
+                os.EX_DATAERR)
+
     print("Benchmark output in %s is valid." % args[0])
     return os.EX_OK