Merge pull request #9322 from apolcyn/deprecate_benchmark_core_lists

Don't configure core lists in in benchmark driver

7 files changed, 15 insertions, 237 deletions

diff --git a/test/cpp/qps/client.h b/test/cpp/qps/client.h
index 18f9778fc6..baa9304cc2 100644
--- a/test/cpp/qps/client.h
+++ b/test/cpp/qps/client.h
@@ -51,7 +51,6 @@
 
 #include "test/cpp/qps/histogram.h"
 #include "test/cpp/qps/interarrival.h"
-#include "test/cpp/qps/limit_cores.h"
 #include "test/cpp/qps/usage_timer.h"
 #include "test/cpp/util/create_test_channel.h"
 
@@ -374,7 +373,7 @@ class ClientImpl : public Client {
   ClientImpl(const ClientConfig& config,
              std::function<std::unique_ptr<StubType>(std::shared_ptr<Channel>)>
                  create_stub)
-      : cores_(LimitCores(config.core_list().data(), config.core_list_size())),
+      : cores_(gpr_cpu_num_cores()),
         channels_(config.client_channels()),
         create_stub_(create_stub) {
     for (int i = 0; i < config.client_channels(); i++) {
diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc
index 93ef32db77..74fe3662c1 100644
--- a/test/cpp/qps/driver.cc
+++ b/test/cpp/qps/driver.cc
@@ -76,30 +76,6 @@ static std::string get_host(const std::string& worker) {
   return s;
 }
 
-static std::unordered_map<string, std::deque<int>> get_hosts_and_cores(
-    const deque<string>& workers) {
-  std::unordered_map<string, std::deque<int>> hosts;
-  for (auto it = workers.begin(); it != workers.end(); it++) {
-    const string host = get_host(*it);
-    if (hosts.find(host) == hosts.end()) {
-      auto stub = WorkerService::NewStub(
-          CreateChannel(*it, InsecureChannelCredentials()));
-      grpc::ClientContext ctx;
-      ctx.set_wait_for_ready(true);
-      CoreRequest dummy;
-      CoreResponse cores;
-      grpc::Status s = stub->CoreCount(&ctx, dummy, &cores);
-      GPR_ASSERT(s.ok());
-      std::deque<int> dq;
-      for (int i = 0; i < cores.cores(); i++) {
-        dq.push_back(i);
-      }
-      hosts[host] = dq;
-    }
-  }
-  return hosts;
-}
-
 static deque<string> get_workers(const string& env_name) {
   char* env = gpr_getenv(env_name.c_str());
   if (!env) {
@@ -210,7 +186,7 @@ std::unique_ptr<ScenarioResult> RunScenario(
     const ClientConfig& initial_client_config, size_t num_clients,
     const ServerConfig& initial_server_config, size_t num_servers,
     int warmup_seconds, int benchmark_seconds, int spawn_local_worker_count,
-    const char* qps_server_target_override, bool configure_core_lists) {
+    const char* qps_server_target_override) {
   // Log everything from the driver
   gpr_set_log_verbosity(GPR_LOG_SEVERITY_DEBUG);
 
@@ -279,9 +255,6 @@ std::unique_ptr<ScenarioResult> RunScenario(
   std::vector<ServerData> servers(num_servers);
   std::unordered_map<string, std::deque<int>> hosts_cores;
 
-  if (configure_core_lists) {
-    hosts_cores = get_hosts_and_cores(workers);
-  }
   for (size_t i = 0; i < num_servers; i++) {
     gpr_log(GPR_INFO, "Starting server on %s (worker #%" PRIuPTR ")",
             workers[i].c_str(), i);
@@ -289,37 +262,9 @@ std::unique_ptr<ScenarioResult> RunScenario(
         CreateChannel(workers[i], InsecureChannelCredentials()));
 
     ServerConfig server_config = initial_server_config;
-    int server_core_limit = initial_server_config.core_limit();
-    int client_core_limit = initial_client_config.core_limit();
-
-    if (configure_core_lists) {
-      string host_str(get_host(workers[i]));
-      if (server_core_limit == 0 && client_core_limit > 0) {
-        // In this case, limit the server cores if it matches the
-        // same host as one or more clients
-        const auto& dq = hosts_cores.at(host_str);
-        bool match = false;
-        int limit = dq.size();
-        for (size_t cli = 0; cli < num_clients; cli++) {
-          if (host_str == get_host(workers[cli + num_servers])) {
-            limit -= client_core_limit;
-            match = true;
-          }
-        }
-        if (match) {
-          GPR_ASSERT(limit > 0);
-          server_core_limit = limit;
-        }
-      }
-      if (server_core_limit > 0) {
-        auto& dq = hosts_cores.at(host_str);
-        GPR_ASSERT(dq.size() >= static_cast<size_t>(server_core_limit));
-        gpr_log(GPR_INFO, "Setting server core_list");
-        for (int core = 0; core < server_core_limit; core++) {
-          server_config.add_core_list(dq.front());
-          dq.pop_front();
-        }
-      }
+    if (server_config.core_limit() != 0) {
+      gpr_log(GPR_ERROR,
+              "server config core limit is set but ignored by driver");
     }
 
     ServerArgs args;
@@ -364,33 +309,8 @@ std::unique_ptr<ScenarioResult> RunScenario(
         CreateChannel(worker, InsecureChannelCredentials()));
     ClientConfig per_client_config = client_config;
 
-    int server_core_limit = initial_server_config.core_limit();
-    int client_core_limit = initial_client_config.core_limit();
-    if (configure_core_lists &&
-        ((server_core_limit > 0) || (client_core_limit > 0))) {
-      auto& dq = hosts_cores.at(get_host(worker));
-      if (client_core_limit == 0) {
-        // limit client cores if it matches a server host
-        bool match = false;
-        int limit = dq.size();
-        for (size_t srv = 0; srv < num_servers; srv++) {
-          if (get_host(worker) == get_host(workers[srv])) {
-            match = true;
-          }
-        }
-        if (match) {
-          GPR_ASSERT(limit > 0);
-          client_core_limit = limit;
-        }
-      }
-      if (client_core_limit > 0) {
-        GPR_ASSERT(dq.size() >= static_cast<size_t>(client_core_limit));
-        gpr_log(GPR_INFO, "Setting client core_list");
-        for (int core = 0; core < client_core_limit; core++) {
-          per_client_config.add_core_list(dq.front());
-          dq.pop_front();
-        }
-      }
+    if (initial_client_config.core_limit() != 0) {
+      gpr_log(GPR_ERROR, "client config core limit set but ignored");
     }
 
     // Reduce channel count so that total channels specified is held regardless
diff --git a/test/cpp/qps/driver.h b/test/cpp/qps/driver.h
index b5c8152e1b..e72d30a4ef 100644
--- a/test/cpp/qps/driver.h
+++ b/test/cpp/qps/driver.h
@@ -46,8 +46,7 @@ std::unique_ptr<ScenarioResult> RunScenario(
     const grpc::testing::ClientConfig& client_config, size_t num_clients,
     const grpc::testing::ServerConfig& server_config, size_t num_servers,
     int warmup_seconds, int benchmark_seconds, int spawn_local_worker_count,
-    const char* qps_server_target_override = "",
-    bool configure_core_lists = true);
+    const char* qps_server_target_override = "");
 
 bool RunQuit();
 }  // namespace testing
diff --git a/test/cpp/qps/limit_cores.cc b/test/cpp/qps/limit_cores.cc
deleted file mode 100644
index b5c222542b..0000000000
--- a/test/cpp/qps/limit_cores.cc
+++ /dev/null
@@ -1,87 +0,0 @@
-/*
- *
- * Copyright 2016, Google Inc.
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
- */
-
-#include "test/cpp/qps/limit_cores.h"
-
-#include <grpc/support/cpu.h>
-#include <grpc/support/log.h>
-#include <grpc/support/port_platform.h>
-
-#ifdef GPR_CPU_LINUX
-#ifndef _GNU_SOURCE
-#define _GNU_SOURCE
-#endif
-#include <sched.h>
-
-namespace grpc {
-namespace testing {
-
-int LimitCores(const int* cores, int cores_size) {
-  const int num_cores = gpr_cpu_num_cores();
-  int cores_set = 0;
-
-  cpu_set_t* cpup = CPU_ALLOC(num_cores);
-  GPR_ASSERT(cpup);
-  const size_t size = CPU_ALLOC_SIZE(num_cores);
-  CPU_ZERO_S(size, cpup);
-
-  if (cores_size > 0) {
-    for (int i = 0; i < cores_size; i++) {
-      if (cores[i] < num_cores) {
-        CPU_SET_S(cores[i], size, cpup);
-        cores_set++;
-      }
-    }
-  } else {
-    for (int i = 0; i < num_cores; i++) {
-      CPU_SET_S(i, size, cpup);
-      cores_set++;
-    }
-  }
-  bool affinity_set = (sched_setaffinity(0, size, cpup) == 0);
-  CPU_FREE(cpup);
-  return affinity_set ? cores_set : num_cores;
-}
-
-}  // namespace testing
-}  // namespace grpc
-#else
-namespace grpc {
-namespace testing {
-
-// LimitCores is not currently supported for non-Linux platforms
-int LimitCores(const int*, int) { return gpr_cpu_num_cores(); }
-
-}  // namespace testing
-}  // namespace grpc
-#endif
diff --git a/test/cpp/qps/limit_cores.h b/test/cpp/qps/limit_cores.h
deleted file mode 100644
index 5482904a3c..0000000000
--- a/test/cpp/qps/limit_cores.h
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- *
- * Copyright 2016, Google Inc.
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
- */
-
-#ifndef TEST_QPS_LIMIT_CORES_H
-#define TEST_QPS_LIMIT_CORES_H
-
-namespace grpc {
-namespace testing {
-/// LimitCores: allow this worker to only run on the cores specified in the
-/// array \a cores, which is of length \a cores_size.
-///
-/// LimitCores takes array and size arguments (instead of vector) for direct
-/// conversion from repeated field of protobuf. Use a cores_size of 0 to remove
-/// existing limits (from an empty repeated field)
-int LimitCores(const int *cores, int cores_size);
-}  // namespace testing
-}  // namespace grpc
-
-#endif  // TEST_QPS_LIMIT_CORES_H
diff --git a/test/cpp/qps/qps_json_driver.cc b/test/cpp/qps/qps_json_driver.cc
index 57ee5ef63c..ddaaa7ca75 100644
--- a/test/cpp/qps/qps_json_driver.cc
+++ b/test/cpp/qps/qps_json_driver.cc
@@ -70,9 +70,6 @@ DEFINE_double(error_tolerance, 0.01,
 DEFINE_string(qps_server_target_override, "",
               "Override QPS server target to configure in client configs."
               "Only applicable if there is a single benchmark server.");
-DEFINE_bool(configure_core_lists, true,
-            "Provide 'core_list' parameters to workers. Value determined "
-            "by cores available and 'core_limit' parameters of the scenarios.");
 
 namespace grpc {
 namespace testing {
@@ -80,12 +77,12 @@ namespace testing {
 static std::unique_ptr<ScenarioResult> RunAndReport(const Scenario& scenario,
                                                     bool* success) {
   std::cerr << "RUNNING SCENARIO: " << scenario.name() << "\n";
-  auto result = RunScenario(
-      scenario.client_config(), scenario.num_clients(),
-      scenario.server_config(), scenario.num_servers(),
-      scenario.warmup_seconds(), scenario.benchmark_seconds(),
-      scenario.spawn_local_worker_count(),
-      FLAGS_qps_server_target_override.c_str(), FLAGS_configure_core_lists);
+  auto result =
+      RunScenario(scenario.client_config(), scenario.num_clients(),
+                  scenario.server_config(), scenario.num_servers(),
+                  scenario.warmup_seconds(), scenario.benchmark_seconds(),
+                  scenario.spawn_local_worker_count(),
+                  FLAGS_qps_server_target_override.c_str());
 
   // Amend the result with scenario config. Eventually we should adjust
   // RunScenario contract so we don't need to touch the result here.
diff --git a/test/cpp/qps/server.h b/test/cpp/qps/server.h
index c3d18e5789..821d5935be 100644
--- a/test/cpp/qps/server.h
+++ b/test/cpp/qps/server.h
@@ -42,7 +42,6 @@
 #include "src/proto/grpc/testing/messages.grpc.pb.h"
 #include "test/core/end2end/data/ssl_test_data.h"
 #include "test/core/util/port.h"
-#include "test/cpp/qps/limit_cores.h"
 #include "test/cpp/qps/usage_timer.h"
 
 namespace grpc {
@@ -51,7 +50,7 @@ namespace testing {
 class Server {
  public:
   explicit Server(const ServerConfig& config) : timer_(new UsageTimer) {
-    cores_ = LimitCores(config.core_list().data(), config.core_list_size());
+    cores_ = gpr_cpu_num_cores();
     if (config.port()) {
       port_ = config.port();