[Openmp-commits] [PATCH] D81054: [OpenMP] Introduce target memory manager

Wed Jul 22 19:03:13 PDT 2020

jdoerfert added a comment.

I left a bunch of comments below, from minor nits and style things to design suggestions. I think high-level there are a few things:

- We want to manage device memory for allocations up to as user defined maximum size. That means we need compile time parameters here, maybe also read environment variables at creation time.
- We should consider a "more complex" designs that allow to trade of wasted memory versus number of allocations. For example, we can allocate an array with N elements of size `S` if we run out of `S`-sized nodes. That further minimizes the number of runtime calls through the entire system. Let's leave that for later though.

================
Comment at: openmp/libomptarget/src/device.cpp:204
+      uintptr_t tp =
+          (uintptr_t)MemoryManager.allocate(Size, HstPtrBegin, DeviceID);
       DP("Creating new map entry: HstBase=" DPxMOD ", HstBegin=" DPxMOD ", "
----------------
Nit: make `tp` a `void *` and cast the one use of it as `uintptr_t` instead.

================
Comment at: openmp/libomptarget/src/device.cpp:285
           DPxPTR(HT.TgtPtrBegin), Size);
-      RTL->data_delete(RTLDeviceID, (void *)HT.TgtPtrBegin);
+      MemoryManager.free((void *)HT.TgtPtrBegin, DeviceID);
       DP("Removing%s mapping with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD
----------------
Nit: Remove the cast.

================
Comment at: openmp/libomptarget/src/memory.cpp:61
+  return L;
+}
+
----------------
No inline but static please. Same above. This feels like something we could use from llvm other share with libomp... this code duplication is a nightmare.

Anyway, as @JonChesterfield mentioned, we should aim for a unit test here. We could also add a executable test case that hits a bucket really hard to ensure we can deal with it.

================
Comment at: openmp/libomptarget/src/memory.cpp:68
+  Node(size_t S, void *P) : Size(S), Ptr(P) {}
+};
+
----------------
Function and member comments in doxygen style please. Also in the header.

---

Add a static assert that the node size is 2 * sizeof(void*).

================
Comment at: openmp/libomptarget/src/memory.cpp:70
+
+class MemoryManagerTy {
+  std::list<Node *> FreeList[NumBuckets];
----------------
Comment explaining this thing. I'm also very confused by the duplicated class declaration. Let's not do that.

================
Comment at: openmp/libomptarget/src/memory.cpp:77
+  std::mutex NodeListLock;
+  DeviceTy *Device;
+
----------------
Comments explaining these things. Maybe place the mutexes next to the things they protext.

Why a std::list and an unordered map? 

Naturally, I would have gone with a `vector` or `std::deque`. To "delete" elements I would mark them taken. There should be 32bit padding in a `Node` anyway. Though hard to predict what is good. 

I am unsure about the map, w/o measurements its guesswork and I would go with the regular one but this is fine.

================
Comment at: openmp/libomptarget/src/memory.cpp:85
+    return Device->RTL->data_delete(Device->RTLDeviceID, Ptr);
+  }
+
----------------
Comments on all of these please. Maye `allocateOnDevice` as name instead? 

================
Comment at: openmp/libomptarget/src/memory.cpp:91
+  // free one or two) and try to allocate again until either we can get memory
+  // or every buffer we hold has been freed.
+  void *freeAndAllocate(size_t Size, void *HstPtr) {
----------------
Hm.. running out of memory seems like a "edge case" and if it happens it seems "likely" it happens again. Why not use the opportunity to free everything in the free list while we are here. I mean, it will be "cheaper", complexity wise, reasonably useful given that the next allocation will hit the same problem, and very much simpler.

================
Comment at: openmp/libomptarget/src/memory.cpp:150
+        deleteFromDevice(N.Ptr);
+  }
+
----------------
If this is part of the device, the place we tear down the context, this issue should go away, I think.

================
Comment at: openmp/libomptarget/src/memory.cpp:153
+  void *allocate(size_t Size, void *HstPtr) {
+    assert(Size && "Size is zero");
+
----------------
At least for malloc and friends that is totally fine btw. If we filter this earlier we can leave the assert though.

================
Comment at: openmp/libomptarget/src/memory.cpp:158
+
+    const int B = findBucket(Size);
+    std::list<Node *> &List = FreeList[B];
----------------
Descriptive variable names are worth the trouble typing.

================
Comment at: openmp/libomptarget/src/memory.cpp:172
+      // Allocate one from device
+      DevPtr = allocateFromDevice(Size, HstPtr);
+
----------------
We should round the size up to increase reuse. Also makes all blocks in a bucket the same size.

================
Comment at: openmp/libomptarget/src/memory.cpp:182
+      if (DevPtr == nullptr)
+        return nullptr;
+
----------------
Nothing is wrong, we just run OOM, return a `nullptr` and all is good.

================
Comment at: openmp/libomptarget/src/memory.cpp:197
+        P = &NodeList.back();
+      }
+    } else {
----------------
As the lock is released, this can/should go into a helper function.

================
Comment at: openmp/libomptarget/src/memory.h:21
+class MemoryManagerTy {
+public:
+  // Allocate memory from device DeviceId
----------------
If there is no private state I'd go for struct. Though I would have expected private state TBH.

================
Comment at: openmp/libomptarget/src/memory.h:35
+
+extern MemoryManagerTy MemoryManager;
----------------
I think the MemoryManager, like the StreamManager, is a thing that belongs to a Device. Different devices might choose different implementations etc. That also reduced our global state footprint. Note that you can and should keep the memory.{h,cpp} files, but make the object part of a Device if possible.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81054/new/

https://reviews.llvm.org/D81054