Bug 197107 – Add script to merge run_benchmark jsons

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add script to merge run_benchmark jsons

[Keith Miller]

Created attachment 367806 [details]
Patch

[Keith Miller]

Created attachment 367807 [details]
Patch

[Michael Saboff]

Comment on attachment 367807 [details]
Patch

r=me

[Dean Johnson]

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)

[Saam Barati]

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

[Keith Miller]

Created attachment 367824 [details]
Patch for landing

[Dean Johnson]

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

[WebKit Commit Bot]

Comment on attachment 367824 [details]
Patch for landing

Clearing flags on attachment: 367824

Committed r244469: <https://trac.webkit.org/changeset/244469>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50059839>