Address github comments

4 files changed, 45 insertions, 19 deletions

diff --git a/tools/jenkins/run_performance.sh b/tools/jenkins/run_performance.sh
index 3673fa6c40..9529b0126f 100755
--- a/tools/jenkins/run_performance.sh
+++ b/tools/jenkins/run_performance.sh
@@ -20,4 +20,4 @@ set -ex
 cd $(dirname $0)/../..
 
 tools/run_tests/start_port_server.py
-test/cpp/qps/qps_diff.py -d origin/$ghprbTargetBranch 
+tools/profiling/qps/qps_diff.py -d origin/$ghprbTargetBranch 
diff --git a/tools/profiling/microbenchmarks/bm_json.py b/tools/profiling/microbenchmarks/bm_json.py
index 930287e0d6..f6082fe7b4 100644
--- a/tools/profiling/microbenchmarks/bm_json.py
+++ b/tools/profiling/microbenchmarks/bm_json.py
@@ -167,7 +167,7 @@ def parse_name(name):
   return out
 
 def expand_json(js, js2 = None):
-  assert(js or js2)
+  if not js and not js2: raise StopIteration()
   if not js: js = js2
   for bm in js['benchmarks']:
     if bm['name'].endswith('_stddev') or bm['name'].endswith('_mean'): continue
diff --git a/test/cpp/qps/qps_diff.py b/tools/profiling/qps/qps_diff.py
index 1dadd96adf..65c845caa1 100755
--- a/test/cpp/qps/qps_diff.py
+++ b/tools/profiling/qps/qps_diff.py
@@ -15,29 +15,26 @@
 # limitations under the License.
 """ Computes the diff between two qps runs and outputs significant results """
 
+import argparse
+import json
+import multiprocessing
+import os
+import qps_scenarios
 import shutil
 import subprocess
-import os
-import multiprocessing
-import json
 import sys
 import tabulate
-import argparse
 
 sys.path.append(
   os.path.join(
-    os.path.dirname(sys.argv[0]), '..', '..', '..', 'tools', 'profiling', 'microbenchmarks', 'bm_diff'))
+    os.path.dirname(sys.argv[0]), '..', 'microbenchmarks', 'bm_diff'))
 import bm_speedup
 
 sys.path.append(
   os.path.join(
-    os.path.dirname(sys.argv[0]), '..', '..', '..', 'tools', 'run_tests', 'python_utils'))
+    os.path.dirname(sys.argv[0]), '..', '..', 'run_tests', 'python_utils'))
 import comment_on_pr
 
-_SCENARIOS = {
-  'large-message-throughput': '{"scenarios":[{"name":"large-message-throughput", "spawn_local_worker_count": -2, "warmup_seconds": 30, "benchmark_seconds": 270, "num_servers": 1, "server_config": {"async_server_threads": 1, "security_params": null, "server_type": "ASYNC_SERVER"}, "num_clients": 1, "client_config": {"client_type": "ASYNC_CLIENT", "security_params": null, "payload_config": {"simple_params": {"resp_size": 1048576, "req_size": 1048576}}, "client_channels": 1, "async_client_threads": 1, "outstanding_rpcs_per_channel": 1, "rpc_type": "UNARY", "load_params": {"closed_loop": {}}, "histogram_params": {"max_possible": 60000000000.0, "resolution": 0.01}}}]}',
-  'multi-channel-64-KiB': '{"scenarios":[{"name":"multi-channel-64-KiB", "spawn_local_worker_count": -3, "warmup_seconds": 30, "benchmark_seconds": 270, "num_servers": 1, "server_config": {"async_server_threads": 31, "security_params": null, "server_type": "ASYNC_SERVER"}, "num_clients": 2, "client_config": {"client_type": "ASYNC_CLIENT", "security_params": null, "payload_config": {"simple_params": {"resp_size": 65536, "req_size": 65536}}, "client_channels": 32, "async_client_threads": 31, "outstanding_rpcs_per_channel": 100, "rpc_type": "UNARY", "load_params": {"closed_loop": {}}, "histogram_params": {"max_possible": 60000000000.0, "resolution": 0.01}}}]}'
-}
 
 def _args():
   argp = argparse.ArgumentParser(
@@ -52,7 +49,7 @@ def _args():
     '--loops',
     type=int,
     default=4,
-    help='Number of times to loops the benchmarks. More loops cuts down on noise'
+    help='Number of loops for each benchmark. More loops cuts down on noise'
   )
   argp.add_argument(
     '-j',
@@ -64,8 +61,10 @@ def _args():
   assert args.diff_base, "diff_base must be set"
   return args
 
+
 def _make_cmd(jobs):
-  return ['make', '-j', '%d' % jobs, 'qps_json_driver', 'qps_worker',]
+  return ['make', '-j', '%d' % jobs, 'qps_json_driver', 'qps_worker']
+
 
 def build(name, jobs):
   shutil.rmtree('qps_diff_%s' % name, ignore_errors=True)
@@ -77,15 +76,18 @@ def build(name, jobs):
     subprocess.check_call(_make_cmd(jobs))
   os.rename('bins', 'qps_diff_%s' % name)
 
+
 def _run_cmd(name, scenario, fname):
   return ['qps_diff_%s/opt/qps_json_driver' % name, '--scenarios_json', scenario, '--json_file_out', fname]
 
+
 def run(name, scenarios, loops):
   for sn in scenarios:
     for i in range(0, loops):
       fname = "%s.%s.%d.json" % (sn, name, i)
       subprocess.check_call(_run_cmd(name, scenarios[sn], fname))
 
+
 def _load_qps(fname):
   try:
     with open(fname) as f:
@@ -97,6 +99,7 @@ def _load_qps(fname):
     print("ValueError occurred reading file: %s" % fname)
     return None
 
+
 def _median(ary):
   assert (len(ary))
   ary = sorted(ary)
@@ -106,6 +109,7 @@ def _median(ary):
   else:
     return ary[n / 2]
 
+
 def diff(scenarios, loops, old, new):
   old_data = {}
   new_data = {}
@@ -133,8 +137,8 @@ def diff(scenarios, loops, old, new):
   else:
     return None
 
-def main(args):
 
+def main(args):
   build('new', args.jobs)
 
   if args.diff_base:
@@ -147,10 +151,10 @@ def main(args):
       subprocess.check_call(['git', 'checkout', where_am_i])
       subprocess.check_call(['git', 'submodule', 'update'])
 
-  run('new', _SCENARIOS, args.loops)
-  run('old', _SCENARIOS, args.loops)
+  run('new', qps_scenarios._SCENARIOS, args.loops)
+  run('old', qps_scenarios._SCENARIOS, args.loops)
 
-  diff_output = diff(_SCENARIOS, args.loops, 'old', 'new')
+  diff_output = diff(qps_scenarios._SCENARIOS, args.loops, 'old', 'new')
 
   if diff_output:
     text = '[qps] Performance differences noted:\n%s' % diff_output
@@ -159,6 +163,7 @@ def main(args):
   print('%s' % text)
   comment_on_pr.comment_on_pr('```\n%s\n```' % text)
 
+
 if __name__ == '__main__':
   args = _args()
-  main(args);
+  main(args)
diff --git a/tools/profiling/qps/qps_scenarios.py b/tools/profiling/qps/qps_scenarios.py
new file mode 100644
index 0000000000..d473d7cd24
--- /dev/null
+++ b/tools/profiling/qps/qps_scenarios.py
@@ -0,0 +1,21 @@
+#!/usr/bin/env python2.7
+#
+# Copyright 2017 gRPC authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+""" QPS Scenarios to run """
+
+_SCENARIOS = {
+  'large-message-throughput': '{"scenarios":[{"name":"large-message-throughput", "spawn_local_worker_count": -2, "warmup_seconds": 30, "benchmark_seconds": 270, "num_servers": 1, "server_config": {"async_server_threads": 1, "security_params": null, "server_type": "ASYNC_SERVER"}, "num_clients": 1, "client_config": {"client_type": "ASYNC_CLIENT", "security_params": null, "payload_config": {"simple_params": {"resp_size": 1048576, "req_size": 1048576}}, "client_channels": 1, "async_client_threads": 1, "outstanding_rpcs_per_channel": 1, "rpc_type": "UNARY", "load_params": {"closed_loop": {}}, "histogram_params": {"max_possible": 60000000000.0, "resolution": 0.01}}}]}',
+  'multi-channel-64-KiB': '{"scenarios":[{"name":"multi-channel-64-KiB", "spawn_local_worker_count": -3, "warmup_seconds": 30, "benchmark_seconds": 270, "num_servers": 1, "server_config": {"async_server_threads": 31, "security_params": null, "server_type": "ASYNC_SERVER"}, "num_clients": 2, "client_config": {"client_type": "ASYNC_CLIENT", "security_params": null, "payload_config": {"simple_params": {"resp_size": 65536, "req_size": 65536}}, "client_channels": 32, "async_client_threads": 31, "outstanding_rpcs_per_channel": 100, "rpc_type": "UNARY", "load_params": {"closed_loop": {}}, "histogram_params": {"max_possible": 60000000000.0, "resolution": 0.01}}}]}'
+}