Bug 197107

Summary: Add script to merge run_benchmark jsons
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dean_johnson, dewei_zhu, msaboff, saam, slewis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Keith Miller 2019-04-19 10:52:14 PDT
Add script to merge run_benchmark jsons
Comment 1 Keith Miller 2019-04-19 10:54:29 PDT
Created attachment 367806 [details]
Patch
Comment 2 Keith Miller 2019-04-19 11:23:24 PDT
Created attachment 367807 [details]
Patch
Comment 3 Michael Saboff 2019-04-19 11:46:21 PDT
Comment on attachment 367807 [details]
Patch

r=me
Comment 4 Dean Johnson 2019-04-19 13:36:34 PDT
Comment on attachment 367807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367807&action=review

Code looks good; small nits on Python coding style.

> Tools/Scripts/merge-result-jsons:31
> +

Nit: PEP8 asks that there is 2 lines in-between imports and function definitions (and 1 for methods).

> Tools/Scripts/merge-result-jsons:34
> +        return json.loads(contents.read())

Nit: You can just use `return json.load(contents)`.

> Tools/Scripts/merge-result-jsons:40
> +    if type(value1) is list:

Nit: Python recommends using `isinstance` rather than comparing types directly:
`if isinstance(value1, list):`

> Tools/Scripts/merge-result-jsons:44
> +    for key in list(value1.keys()) + list(value2.keys()):

value1.keys() and value2.keys() already return lists. This can be written a bit more concisely as:
`for key in (value1.keys() + value2.keys()):`

> Tools/Scripts/merge-result-jsons:62
> +        help="File to put the merged json into prints to standard out if nothing is passed")

Nit: You're missing positional arguments for the list of json files. I guess you can just use the remaining args instead, but it'd be more clear to use argparse.
https://docs.python.org/2/library/argparse.html

This should be able to be implemented pretty easily. I think it would look something like:
parser.add_argument('json_files', metavar='N', default=[], nargs='+',
                    help='json files to merge')

> Tools/Scripts/merge-result-jsons:72
> +            f.write(json.dumps(result))

This can be written more concisely:
with open(opts.o, 'w') as f:
     json.dump(result, f)
Comment 5 Saam Barati 2019-04-19 13:40:19 PDT
Comment on attachment 367807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367807&action=review

> Tools/ChangeLog:11
> +        This patch also adds +x bits to the compare-results script so it

Nice. I thought I did this but have also been annoyed by +x not being there. I must've landed through commit queue or something.

> Tools/Scripts/compare-results:236
> +

revert
Comment 6 Keith Miller 2019-04-19 14:06:03 PDT
Created attachment 367824 [details]
Patch for landing
Comment 7 Dean Johnson 2019-04-19 14:31:41 PDT
Comment on attachment 367824 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=367824&action=review

> Tools/Scripts/merge-result-jsons:49
> +

Nit: Missing newlines between each function definition.

> Tools/Scripts/merge-result-jsons:72
> +    result = mergeJSONs(list(map(readJSONFile, args.jsons)))

Nit: `map` already returns a list. No need for an extra conversion
Comment 8 WebKit Commit Bot 2019-04-19 14:35:14 PDT
Comment on attachment 367824 [details]
Patch for landing

Clearing flags on attachment: 367824

Committed r244469: <https://trac.webkit.org/changeset/244469>
Comment 9 WebKit Commit Bot 2019-04-19 14:35:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-04-19 14:36:16 PDT
<rdar://problem/50059839>