Proper heterogen execution modes #652

akroviakov · 2023-08-25T09:39:16Z

This patch integrates the device multifragment policy (currently disabled) in its proper form for heterogeneous kernels, that is (depending on query):

CPU: kernel per fragment
GPU: kernel per N fragments

kurapov-peter · 2023-08-25T19:45:27Z

omniscidb/QueryEngine/Execute.cpp

@@ -2790,66 +2790,109 @@ std::vector<std::unique_ptr<ExecutionKernel>> Executor::createHeterogeneousKerne

  CHECK(!ra_exe_unit.input_descs.empty());

+  const bool use_multifrag_kernel = eo.allow_multifrag && is_agg;


This reads as group-bys would not use a multi-fragment policy. Needs cleanup I guess.

kurapov-peter · 2023-08-25T19:45:44Z

omniscidb/QueryEngine/Execute.cpp

-                << " for kernel per fragment execution path.";
-      throw CompilationRetryNewScanLimit(max_frag_size);
+  if (use_multifrag_kernel) {
+    LOG(INFO) << "use_multifrag_kernel=" << use_multifrag_kernel;


This is debug information. Same for others.

kurapov-peter · 2023-08-25T19:49:07Z

omniscidb/QueryEngine/Descriptors/QueryFragmentDescriptor.h

@@ -100,6 +100,56 @@ class QueryFragmentDescriptor {
    }
  }

+  template <typename DISPATCH_FCN>
+  void assignFragsToMultiHeterogeneousDispatch(


Having a separate method is a bit ugly (that was the reason we didn't merge it long time ago), but I think we can live with that for now.

kurapov-peter

Dispatching becomes hard to follow, I think it needs more abstractions/simplifications.

omniscidb/QueryEngine/Execute.cpp

kurapov-peter · 2023-08-28T11:20:08Z

omniscidb/QueryEngine/Execute.cpp

-  const auto device_count = deviceCount(device_type);
+  const int device_count = config_->exec.heterogeneous.enable_heterogeneous_execution
+                               ? available_cpus + available_gpus.size()
+                               : deviceCount(query_mem_descs.begin()->first);


This looks error-prone. Maybe a least add a check for the case when we expect only one query memory descriptor to be present in the map.

The device_count differed before also only by this flag, but I can add a check :). It was also passed as a parameter with no real goal. Do we even need it except for >0 check?

You can remove it if you make sure a device is always available.

kurapov-peter · 2023-08-28T11:22:55Z

omniscidb/QueryEngine/Execute.cpp

+    // execution_kernels_per_device_.
+    const ExecutorDeviceType intended_dt = intended_dt_itr.first;
+    const ExecutorDeviceType actual_dt = intended_dt_itr.second->getDeviceType();
+    LOG(INFO) << "Query was inteded for " << intended_dt << ", will actually run on "


What are the situations in which we would want to reschedule some (not all) of the kernels to the CPU? In other words, do we need such logic at all? Maybe have a fallback policy instead?

kurapov-peter · 2023-08-28T11:29:25Z

omniscidb/QueryEngine/Execute.cpp

+              << actual_dt;
+
+    if (actual_dt == ExecutorDeviceType::GPU && eo.allow_multifrag &&
+        (!uses_lazy_fetch || is_agg)) {


Where does this condition come from? And what do these parameters have to do with multi-fragment dispatching?

They were there previously. And when they are ignored we have validation issues on some tests, so I guess they are there for a reason.

kurapov-peter · 2023-08-28T11:36:58Z

omniscidb/QueryEngine/Execute.cpp

+    if (actual_dt == ExecutorDeviceType::GPU && eo.allow_multifrag &&
+        (!uses_lazy_fetch || is_agg)) {
+      policy->devices_dispatch_modes.at(intended_dt) =
+          ExecutorDispatchMode::MultifragmentKernel;


I think we should not modify the policy during the scheduling. The policy is decided by the cost model is then used by the scheduling algorithm to distribute work.

kurapov-peter · 2023-08-28T11:43:30Z

omniscidb/QueryEngine/Descriptors/QueryFragmentDescriptor.h

+        bool dispatch_finished = false;
+        while (!dispatch_finished) {
+          dispatch_finished = true;
+          for (const auto& device_type_itr : execution_kernels_per_device_)


What's the point of this inner loop when you have the very same outer one?

kurapov-peter · 2023-08-28T11:49:12Z

omniscidb/QueryEngine/Descriptors/QueryFragmentDescriptor.h

+          for (const auto& device_type_itr : execution_kernels_per_device_)
+            for (const auto& device_itr : device_type_itr.second) {
+              auto& kernel_idx =
+                  execution_kernel_index[device_type_itr.first][device_itr.first];


Why populate the execution_kernel_index at all? Can't you just merge the two loops?

Because we are inside the while (!dispatch_finished){} loop, so we may actually revisit for(const auto& device_itr : device_type_itr.second){}, but to not schedule already scheduled kernels, we keep track of the last scheduled kernel idx to continue from that index on

kurapov-peter · 2023-09-01T14:41:10Z

omniscidb/QueryEngine/Execute.cpp

-      query_mem_descs_owned.insert(std::make_pair(dt, std::move(query_mem_desc_owned)));
+      const ExecutorDeviceType compiled_for_dt{query_comp_desc_owned->getDeviceType()};
+      if (!query_comp_descs_owned.count(
+              compiled_for_dt)) {  // Can a fallback to CPU generate different kernels?


Let's keep the latest compilation (after the fallback)

kurapov-peter

Seems OK, some minor comments.

kurapov-peter · 2023-09-04T11:03:39Z

omniscidb/QueryEngine/CostModel/Dispatchers/ProportionBasedExecutionPolicy.cpp

@@ -19,7 +19,9 @@
 namespace policy {

 ProportionBasedExecutionPolicy::ProportionBasedExecutionPolicy(
-    std::map<ExecutorDeviceType, unsigned>&& propotion) {
+    std::map<ExecutorDeviceType, unsigned>&& propotion,


kurapov-peter · 2023-09-04T11:14:03Z

omniscidb/QueryEngine/Execute.cpp

-
+  auto device_types_for_query = getDeviceTypesForQuery(
+      ra_exe_unit, query_infos, co.device_type, max_groups_buffer_entry_guess, eo);
+  CHECK(device_types_for_query.size());


Suggested change

CHECK(device_types_for_query.size());

CHECK_GT(device_types_for_query.size(), size_t(0));

kurapov-peter · 2023-09-04T11:15:09Z

omniscidb/QueryEngine/Execute.cpp

+    const auto exe_policy =
+        getExecutionPolicy(is_agg, query_mem_descs_owned, ra_exe_unit, query_infos, eo);
+    const ExecutorDeviceType fallback_device{
+        exe_policy->hasDevice(co.device_type) ? co.device_type : ExecutorDeviceType::CPU};


Shouldn't this be always CPU?

Right now main has it this way: can return requested_device_type (i.e., co.device_type) as fallback_device. Is current main wrong?

kurapov-peter · 2023-09-04T11:16:34Z

omniscidb/QueryEngine/Execute.cpp

+          devices_count += get_available_gpus(data_mgr_).size();
+        }
+      }
+      CHECK(devices_count);


Suggested change

CHECK(devices_count);

CHECK_GT(devices_count, size_t(0));

kurapov-peter · 2023-09-04T11:21:08Z

omniscidb/QueryEngine/CostModel/Dispatchers/ExecutionPolicy.h

+  // modes.
+  const std::map<ExecutorDeviceType, ExecutorDispatchMode> devices_dispatch_modes_{
+      {ExecutorDeviceType::CPU, ExecutorDispatchMode::KernelPerFragment},
+      {ExecutorDeviceType::GPU, ExecutorDispatchMode::KernelPerFragment}};


Since these are set in constructors it seems there is no reason to have the default values here (unless we use default constructors somewhere which is likely incorrect). The comment together with const is confusing.

kurapov-peter · 2023-09-04T11:34:48Z

omniscidb/QueryEngine/Execute.cpp

+  for (const auto& dt_query_desc : query_mem_descs) {
+    if (policy->getExecutionMode(dt_query_desc.first) ==
+        ExecutorDispatchMode::KernelPerFragment) {
+      VLOG(1) << "Creating one execution kernel per fragment";


Looks like the log is out of place. Mb move it to where kernels are actually created.

kurapov-peter

LGTM!

akroviakov requested a review from kurapov-peter August 25, 2023 09:39

akroviakov force-pushed the akroviak/heterogen_scheduling branch from 3e72fd7 to ad43824 Compare August 25, 2023 13:00

enable multifrag

ad43824

kurapov-peter reviewed Aug 25, 2023

View reviewed changes

Unify kernel creation & dispatch + heterogen flag

2e963de

kurapov-peter suggested changes Aug 28, 2023

View reviewed changes

akroviakov force-pushed the akroviak/heterogen_scheduling branch 4 times, most recently from b5e1067 to 4980599 Compare August 31, 2023 11:32

akroviakov requested a review from kurapov-peter September 1, 2023 07:02

akroviakov changed the title ~~Proper multifrag~~ Proper heterogen execution modes Sep 1, 2023

kurapov-peter reviewed Sep 1, 2023

View reviewed changes

akroviakov force-pushed the akroviak/heterogen_scheduling branch from 4980599 to 1784a5d Compare September 1, 2023 16:07

kurapov-peter reviewed Sep 4, 2023

View reviewed changes

akroviakov force-pushed the akroviak/heterogen_scheduling branch from 1784a5d to 89478b7 Compare September 12, 2023 07:48

unambiguous executor

89478b7

akroviakov requested a review from kurapov-peter September 12, 2023 14:41

kurapov-peter approved these changes Sep 20, 2023

View reviewed changes

kurapov-peter merged commit ff6b55e into main Sep 26, 2023

kurapov-peter deleted the akroviak/heterogen_scheduling branch September 26, 2023 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper heterogen execution modes #652

Proper heterogen execution modes #652

akroviakov commented Aug 25, 2023 •

edited

Loading

kurapov-peter Aug 25, 2023

kurapov-peter Aug 25, 2023

kurapov-peter Aug 25, 2023

kurapov-peter left a comment

kurapov-peter Aug 28, 2023

akroviakov Aug 28, 2023

kurapov-peter Aug 28, 2023

kurapov-peter Aug 28, 2023

kurapov-peter Aug 28, 2023

akroviakov Aug 28, 2023

kurapov-peter Aug 28, 2023

kurapov-peter Aug 28, 2023

kurapov-peter Aug 28, 2023

akroviakov Aug 28, 2023

kurapov-peter Sep 1, 2023

kurapov-peter left a comment

kurapov-peter Sep 4, 2023

kurapov-peter Sep 4, 2023

kurapov-peter Sep 4, 2023

akroviakov Sep 10, 2023

kurapov-peter Sep 4, 2023

kurapov-peter Sep 4, 2023

kurapov-peter Sep 4, 2023

kurapov-peter left a comment

		@@ -2790,66 +2790,109 @@ std::vector<std::unique_ptr<ExecutionKernel>> Executor::createHeterogeneousKerne

		CHECK(!ra_exe_unit.input_descs.empty());

		const bool use_multifrag_kernel = eo.allow_multifrag && is_agg;

	CHECK(device_types_for_query.size());
	CHECK_GT(device_types_for_query.size(), size_t(0));

Proper heterogen execution modes #652

Proper heterogen execution modes #652

Conversation

akroviakov commented Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurapov-peter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurapov-peter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurapov-peter left a comment

Choose a reason for hiding this comment

akroviakov commented Aug 25, 2023 •

edited

Loading