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

Tang, Jun 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
Tang, Jun 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