[Graph Partitioning] Add optimization to minimize communication cost and number of partitions. #2359

beicy · 2019-02-06T20:25:56Z

Description:
Based on the initial partitions, this PR adds:

Adjust the initial partition (moving nodes from one partition to another partition) to minimize the communication cost.
Combine the partitions if necessary.

Testing:
For unittest PartitionerTest::Basic1, the communication cost is 288 bytes->256 bytes.
For PartitionerTest::Basic2, the communication cost is 160 bytes->64 bytes. And the number of partitions is 3 -> 2. The following 2 pics shows the initial partition and optimized partitions:
The initial partition ( Memory 2048 bytes. 3 partitions, with memory (1152 bytes, 1152 bytes, 1376 bytes)

The optimized partition (Memory 2048 bytes. 2 partitions, with memory (1728, 1728) :

Documentation: #2298
[Optional Fixes #issue]

Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

opti-mix · 2019-02-08T01:38:03Z

include/glow/Partitioner/Partitioner.h

@@ -30,19 +31,30 @@ namespace glow {
 using namespace runtime;

 using MemUsageMap = std::unordered_map<Node *, unsigned>;
+using NodesSet = std::set<Node *>;


Minor nit: But I think we tend to call most type aliases something like TypeXyzTy, i.e. Ty at the end.

opti-mix · 2019-02-08T01:39:02Z

include/glow/Partitioner/Partitioner.h

@@ -102,6 +124,19 @@ class Partitioner {
  /// function.
  void initOpMemUsage();

+  /// Combine the partitions if necessary : if all outside uses of the nodes in
+  /// /// partition1 is in partition2, and the sum of memory comsuption of


Two /// are used here.

typo: comsuption -> consumption

opti-mix · 2019-02-08T01:40:14Z

lib/Partitioner/PartitionUtils.cpp

+namespace glow {
+
+/// Given \p nodes, return a list of nodes who use any node in this set.
+std::vector<Node *> getOutUsers(std::set<Node *> nodes) {


Do you really need to pass nodes by value? Why not const std::set<Node *> &?

opti-mix · 2019-02-08T01:40:23Z

lib/Partitioner/PartitionUtils.cpp

+
+/// Given \p nodes, return a list of nodes who use only the nodes in this set or
+/// constant.
+std::vector<Node *> getOutUsersWithOnePre(std::set<Node *> nodes) {


Do you really need to pass nodes by value? Why not const std::set<Node *> &?

In getOutUsersWithOnePre what does the Pre part means? Predecessor? May be it should be a bit longer than Pre to be more descriptive?

Yes, it means "Predecessor" :)

opti-mix · 2019-02-08T02:06:04Z

lib/Partitioner/PartitionUtils.cpp

+    Node *cur = *it;
+    // For Save onde, the only required memory is for output.
+    if (cur->getKind() == Kinded::Kind::SaveNodeKind) {
+      SaveNode *tmp = (SaveNode *)cur;


Use llvm::cast instead of C-style cast.

Or even better:

if (auto *SN = llvm::dyn_cast<SaveNode>(cur)) {...}

opti-mix · 2019-02-08T02:16:46Z

lib/Partitioner/Partitioner.cpp

+        continue;
+      }
+      Function *cur = (*it).first;
+      uint64_t mem =


May be call it memCost?

Or memSize

opti-mix · 2019-02-08T02:17:54Z

lib/Partitioner/Partitioner.cpp

+        if (memUsage_[outUsers[i]] + mem > availableMemory) {
+          continue;
+        }
+        // Rule 2: this move won't cause constant dupication.


Question: May be it is OK to have constants duplication if the size of a constant is below a certain threshold?

typo: dupication -> duplication

Question: May be it is OK to have constants duplication if the size of a constant is below a certain threshold?

@opti-mix Theoretically, it should be allowed if it reduce the communication cost a lot or good for workload balance. Will think about it for the next step. Thanks!

opti-mix · 2019-02-08T02:18:45Z

lib/Partitioner/Partitioner.cpp

+        if (cont) {
+          continue;
+        }
+        // Rule 3: this move won't increase communication cost. Even this move


s/Even this/Even if this/

opti-mix · 2019-02-08T02:19:28Z

lib/Partitioner/Partitioner.cpp

+        // or paritionCombine.
+        nSet.insert(outUsers[i]);
+        GraphMemInfo cost = getGraphMemInfo(nSet);
+        if (cost.outMem <= communicationCost) {


again, may be outMemSize or outMemCost instead of outMem?

opti-mix · 2019-02-08T02:20:12Z

lib/Partitioner/Partitioner.cpp

+          if (nodesSet[suc].empty()) {
+            // It is possible that after moving a node from Partition2 to
+            // Partition1, Partition2 become empty. Remove the empty partition.
+            partitions.deletePartition(suc);


Here you could reuse the partition removal logic if it would be a separate method.

here nodesSet is not partitions's member, and this is just one circumstance under which a partition need to be removed. Therefore, I only add deletePartition as a method.

jfix71 · 2019-02-08T01:37:07Z

lib/Partitioner/Partitioner.cpp

+    partitions.setCost((*it).first, cost);
+  }
+
+  // Move/Exchange nodes between any two connected partions, until no gain is


nit: partions -> partitions

jfix71 · 2019-02-08T02:23:22Z

lib/Partitioner/Partitioner.cpp

@@ -354,12 +495,14 @@ DAGNodeList &Partitioner::Partition() {

  doPartitioning(F_, partitionMap);

+  F_->dumpDAG("f1.dot");


stale debug code

jfix71 · 2019-02-08T02:23:26Z

lib/Partitioner/Partitioner.cpp

  // Remove the original function after partitioning.
  module_->eraseFunction(F_);

  auto funcList = module_->getFunctions();
  for (Function *F : funcList) {
    (void)F;
+    F->dumpDAG("f2.dot");


stale debug code

jfix71 · 2019-02-08T02:25:59Z

lib/Partitioner/CMakeLists.txt

@@ -1,5 +1,6 @@
 add_library(Partitioner
-            Partitioner.cpp)
+	      PartitionUtils.cpp


major nit, but: can we rename PartitionUtils.{cpp, h} to PartitionerUtils.{cpp, h}? Just for consistency in other file names, since we have Partitioner.{cpp, h}, PartitionerTest.cpp, etc.

jfix71 · 2019-02-08T02:26:19Z

include/glow/Partitioner/PartitionUtils.h

+/// The memory usage of a subgraph (i.e. a list of nodes of a function).
+struct GraphMemInfo {
+  // The memory usage of all input nodes (whose predecessors are not included in
+  // this subgraph) of this subgraph


nit: add period at end of comment.

jfix71 · 2019-02-08T02:33:23Z

lib/Partitioner/PartitionUtils.cpp

+
+#include "glow/Partitioner/PartitionUtils.h"
+
+#include <iostream>


Do you need this?

jfix71 · 2019-02-08T02:53:03Z

lib/Partitioner/Partitioner.cpp

+        if (memUsage_[outUsers[i]] + mem > availableMemory) {
+          continue;
+        }
+        // Rule 2: this move won't cause constant dupication.


typo: dupication -> duplication

jfix71 · 2019-02-08T02:59:59Z

lib/Partitioner/Partitioner.cpp

+  // Step1 Move: Assume Partition1 -> Partition2, try to move nodes from
+  // Partition2 to Partition1 if those nodes only use the nodes in
+  // Partition1(recursively) and the move won't make Partition1's memory exceeds
+  // the memory constratint, and the communication cost is minimized.


typo: constratint -> constraint

jfix71 · 2019-02-08T03:15:05Z

lib/Partitioner/Partitioner.cpp

+        }
+        // Rule 3: this move won't increase communication cost. Even this move
+        // won't change communication cost, according to rule 1 and rule 2, the
+        // memory comsuption of the partition where this node (i.e outUsers[i])


typo: comsuption -> consumption

jfix71 · 2019-02-08T03:15:18Z

lib/Partitioner/Partitioner.cpp

@@ -139,6 +139,147 @@ static BFSLevel getBFSLevel(Function *F) {
  return bfs;
 }

+// Combine the partitions if necessary : if all outside uses of the nodes in
+// partition1 is in partition2, and the sum of memory comsuption of partition1


typo: comsuption -> consumption

gcatron

This looks great! Approved pending addressing comments from @opti-mix and @jfix71.

gcatron · 2019-02-08T20:30:49Z

lib/Partitioner/PartitionUtils.cpp

+
+namespace glow {
+
+/// Given \p nodes, return a list of nodes who use any node in this set.


This is really any nodes who use this set that are not in this set right?

…and number of partitions.

facebook-github-bot added the CLA Signed label Feb 6, 2019

beicy force-pushed the Partition branch from d6f3be6 to 4a6493f Compare February 6, 2019 23:59

rdzhabarov assigned gcatron Feb 8, 2019

opti-mix reviewed Feb 8, 2019

View reviewed changes

jfix71 reviewed Feb 8, 2019

View reviewed changes

beicy force-pushed the Partition branch from 4a6493f to a9328e5 Compare February 8, 2019 19:37

gcatron approved these changes Feb 8, 2019

View reviewed changes

[Graph Partitioning] Add optimization to minimize communication cost …

ea39ebf

…and number of partitions.

beicy force-pushed the Partition branch from a9328e5 to ea39ebf Compare February 8, 2019 21:12

beicy merged commit 517f4de into pytorch:master Feb 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Graph Partitioning] Add optimization to minimize communication cost and number of partitions. #2359

[Graph Partitioning] Add optimization to minimize communication cost and number of partitions. #2359

beicy commented Feb 6, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

jfix71 Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

beicy Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

jfix71 Feb 8, 2019

beicy Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

opti-mix Feb 8, 2019

beicy Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

jfix71 Feb 8, 2019

gcatron left a comment

gcatron Feb 8, 2019

beicy Feb 8, 2019

		@@ -354,12 +495,14 @@ DAGNodeList &Partitioner::Partition() {

		doPartitioning(F_, partitionMap);

		F_->dumpDAG("f1.dot");


		#include "glow/Partitioner/PartitionUtils.h"

		#include <iostream>


		namespace glow {

		/// Given \p nodes, return a list of nodes who use any node in this set.

[Graph Partitioning] Add optimization to minimize communication cost and number of partitions. #2359

[Graph Partitioning] Add optimization to minimize communication cost and number of partitions. #2359

Conversation

beicy commented Feb 6, 2019

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

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

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

gcatron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment