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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 4 inline comments as done.
cryptoad added inline comments.


================
Comment at: lib/scudo/standalone/local_cache.h:78
+    }
+    const uptr ClassSize = C->ClassSize;
+    void *P = C->Chunks[--C->Count];
----------------
morehouse wrote:
> Maybe just use `C->ClassSize` directly below.
So this is a memory access trick. PerClassArray is accessed via Count first, and ClassSize is close to Count, while Chunks can be further off if Count is large.
This way we get ClassSize loaded in a register (or enticing the compiler to at least), before accessing Chunks.
I'll add a comment to explain this.


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


================
Comment at: lib/scudo/standalone/local_cache.h:151
+    initCacheMaybe(C);
+    TransferBatch *B = Allocator->popBatch(this, ClassId);
+    DCHECK_GT(B->getCount(), 0);
----------------
morehouse wrote:
> (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.
Ack, good idea, thanks!


================
Comment at: lib/scudo/standalone/local_cache.h:152
+    TransferBatch *B = Allocator->popBatch(this, ClassId);
+    DCHECK_GT(B->getCount(), 0);
+    B->copyToArray(C->Chunks);
----------------
morehouse wrote:
> What if `B == nullptr`?
Batch creation failure is fatal, I'll add a CHECK.


================
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);
----------------
morehouse wrote:
> What if `B == nullptr`?
Same.


================
Comment at: lib/scudo/standalone/primary32.h:76
+                        (I != SizeClassMap::BatchClassId) &&
+                        (getSizeByClassId(I) >= (PageSize / 32));
+    }
----------------
morehouse wrote:
> Why can't we release small size classes?
It's a tradeoff. Since we randomize allocations, smaller class sizes are likelier to be fragmented between allocated/available chunks, making them harder to be entirely empty.
On top of it, smaller size classes require more computing power (we iterate through more chunks).
The net benefit of trying to release smaller classes is not obvious in terms of lowering the RSS, hence the lower (arbitrary limit).
I want at some point to make that configurable via options.
I'll add some comments.


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


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


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


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D61745