[webkit-reviews] review granted: [Bug 93690] run-perf-tests should upload memory statistics to perf-o-matic : [Attachment 157645] Adds the feature

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 93690: run-perf-tests should upload memory statistics to perf-o-matic
https://bugs.webkit.org/show_bug.cgi?id=93690

Attachment 157645: Adds the feature
https://bugs.webkit.org/attachment.cgi?id=157645&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157645&action=review


looks reasonable. As a general warning, I start to get leery these days when we
have to write code that does a lot of free-text parsing like this. Is the text
primarily meant to be human-readable, or is it just going from bots to
dashboards? If the latter, would it make sense to switch to JSON or something
more easily parsed and structured?

Or, if it is used both ways, does it make sense to start with json and then
pretty-print it only when necessary?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:102
> +		   help="Path to generate a JSON file at; may contain previous
results if it already exists."),

This means that we append or merge the new results into the existing results?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:104
> +		   help="Only used on bots. Path to a JSON file to be merged
into the JSON file when --output-json-path is present."),

Do we need both mechanisms for this?

>>> Tools/Scripts/webkitpy/performance_tests/perftest.py:171
>>> +	     _log.info('RESULT %s= %s %s' % (test_name.replace(':', ':
').replace('/', ': '), results['avg'], unit))
>> 
>> I would use re.sub("[:/]", ": ", test_name) or re.compile [:/] + re.sub
>> instead of test_name.replace(':', ': ').replace('/', ': ')
> 
> I think the regular expression is actually slower.

I can't imagine speed is an issue here; I would be more concerned with clarity.
I think the re.sub() is slightly clearer, but only slightly.