[PATCH] D61745: [scudo][standalone] Introduce the Primary(s) and LocalCache

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: lib/scudo/standalone/local_cache.h:78
+    }
+    const uptr ClassSize = C->ClassSize;
+    void *P = C->Chunks[--C->Count];
----------------
Maybe just use `C->ClassSize` directly below.


================
Comment at: lib/scudo/standalone/local_cache.h:95
+      drain(C, ClassId);
+    const uptr ClassSize = C->ClassSize;
+    C->Chunks[C->Count++] = P;
----------------
Again, this line seems unnecessary.


================
Comment at: lib/scudo/standalone/local_cache.h:149
+
+  NOINLINE bool refill(PerClass *C, uptr ClassId) {
+    initCacheMaybe(C);
----------------
We always return true, so why not void?


================
Comment at: lib/scudo/standalone/local_cache.h:151
+    initCacheMaybe(C);
+    TransferBatch *B = Allocator->popBatch(this, ClassId);
+    DCHECK_GT(B->getCount(), 0);
----------------
(Future improvement)
Why do we need to pass `this` to the allocator here?  We just copy the returned batch to `C->Chunks` and destroy it anyway.  Why not directly pass the `PerClass` object and have `popBatch` populate it?  This could be faster and we can break the call graph cycle here.


================
Comment at: lib/scudo/standalone/local_cache.h:152
+    TransferBatch *B = Allocator->popBatch(this, ClassId);
+    DCHECK_GT(B->getCount(), 0);
+    B->copyToArray(C->Chunks);
----------------
What if `B == nullptr`?


================
Comment at: lib/scudo/standalone/local_cache.h:162
+    const uptr FirstIndexToDrain = C->Count - Count;
+    TransferBatch *B = createBatch(ClassId, C->Chunks[FirstIndexToDrain]);
+    B->setFromArray(&C->Chunks[FirstIndexToDrain], Count);
----------------
What if `B == nullptr`?


================
Comment at: lib/scudo/standalone/primary32.h:76
+                        (I != SizeClassMap::BatchClassId) &&
+                        (getSizeByClassId(I) >= (PageSize / 32));
+    }
----------------
Why can't we release small size classes?


================
Comment at: lib/scudo/standalone/primary64.h:120
+  void enable() {
+    for (sptr I = (sptr)NumClasses - 1; I >= 0; I--)
+      getRegionInfo(I)->Mutex.unlock();
----------------
static_cast


================
Comment at: lib/scudo/standalone/tests/primary_test.cc:46
+  Allocator->printStats();
+  delete Allocator;
+}
----------------
`unique_ptr` or allocate on stack


================
Comment at: lib/scudo/standalone/tests/primary_test.cc:121
+  Allocator->printStats();
+  delete Allocator;
+}
----------------
unique_ptr or allocate on stack


================
Comment at: lib/scudo/standalone/tests/primary_test.cc:172
+  Allocator->printStats();
+  delete Allocator;
+}
----------------
unique_ptr or allocate on stack


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D61745