Bug 93690

Created attachment 157645 [details] Adds the feature

Comment on attachment 157645 [details] Adds the feature View in context: https://bugs.webkit.org/attachment.cgi?id=157645&action=review The patch looks okay to me, 2 comments: > 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('/', ': ') > Tools/ChangeLog:12 > + some helps in PerfTest.parse_output per arv's comments. arv's?

Comment on attachment 157645 [details] Adds the feature View in context: https://bugs.webkit.org/attachment.cgi?id=157645&action=review >> 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. >> Tools/ChangeLog:12 >> + some helps in PerfTest.parse_output per arv's comments. > > arv's? arv@chromium

Comment on attachment 157645 [details] Adds the feature 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.

Comment on attachment 157645 [details] Adds the feature View in context: https://bugs.webkit.org/attachment.cgi?id=157645&action=review >> 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? Yes. >> 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? Not really. We can do a follow up to get rid of it but that'll require a build bot change so I'll do that in a separate patch at least.

(In reply to comment #4) > (From update of attachment 157645 [details]) > 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? It's meant to be human readable. The results are uploaded to perf-o-matic as a JSON file. The code in webkitpy is mainly about parsing the DRT result and then converting it to a JSON entry and re-formatting it for stdout. Historically, this was done to be compatible with Chromium perf bots (inherited from loislo's original inspector perf. test framework). However, given that we're not intending to deploy this on Chromium perf bots in the foreseeable future maybe we can yank that and do something simpler.

Committed r125332: <http://trac.webkit.org/changeset/125332>

(In reply to comment #5) > (From update of attachment 157645 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157645&action=review > > >> 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? > > Not really. We can do a follow up to get rid of it but that'll require a build bot change so I'll do that in a separate patch at least. I take it back. It needs to be separate because on bots, we can't keep merging new results and it's hard to distinguish which part of the output needs to be merged to new entry once it got merged.