[PATCH] D37082: [sanitizer] Re-introduce kUseSeparateSizeClassForBatch for the 32-bit Primary

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_local_cache.h:131
 struct SizeClassAllocator32LocalCache {
   typedef SizeClassAllocator Allocator;
   typedef typename Allocator::TransferBatch TransferBatch;
----------------
alekseyshl wrote:
> Do we really need all these types and consts to be public?
That's an interesting point.
You can see below that the `private:` was commented out. I am not sure when or why, but it's all public.
Do you want me to try and reintroduce it an see what we can put in there?


================
Comment at: lib/sanitizer_common/sanitizer_allocator_local_cache.h:215
+      // kBatchClassID, except for kBatchClassID itself.
+      if (kUseSeparateSizeClassForBatch)
+        c->batch_class_id = (i == kBatchClassID) ? 0 : kBatchClassID;
----------------
alekseyshl wrote:
> Add {} around in and else parts.
Will do.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:103
   static uptr ClassIdToSize(uptr class_id) {
-    return SizeClassMap::Size(class_id);
+    return (class_id == SizeClassMap::kBatchClassID) ?
+        kBatchSize : SizeClassMap::Size(class_id);
----------------
alekseyshl wrote:
> Shouldn't it be also guarded by kUseSeparateSizeClassForBatch flag?
In my opinion, even when not using a separate size class for batches, this should return `kBatchSize` for consistency.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_size_class_map.h:137
   static const uptr kNumClasses =
-      kMidClass + ((kMaxSizeLog - kMidSizeLog) << S) + 1;
+      kMidClass + ((kMaxSizeLog - kMidSizeLog) << S) + 1 + 1;
   static const uptr kLargestClassID = kNumClasses - 2;
----------------
alekseyshl wrote:
> So now we have two unused classes at the top? Why kLargestClassID was not adjusted then? Many other places depend on kNumClasses, including asan_allocator, I'm feeling not that easy about this change, how these places are affected?
`kLargestClassID` was indeed not adjusted after the removal of `kBatchClassID`. It should have been `kNumClasses - 1`, but was left `- 2`.
It ended up not having any impact as it was only used in a test (`SizeClassAllocatorGetBlockBeginStress`), which means we didn't stress the last class.
In the end there are 2 "unused" classes when not using separate batches: 0 and kBatchClassID.
I tried to repurpose class 0, but ended up hitting some snags (for example in a ByteMap 0 means not-ours as opposed to being region 0), so I went back to getting an extra class past the end.

Here is a sample output of a `SizeClassMap::Print()`:
```
c00 => s: 0 diff: +0 00% l 0 cached: 0 0; id 0
c01 => s: 16 diff: +16 00% l 4 cached: 64 1024; id 1
c02 => s: 32 diff: +16 100% l 5 cached: 64 2048; id 2
c03 => s: 48 diff: +16 50% l 5 cached: 64 3072; id 3
...
c50 => s: 98304 diff: +16384 20% l 16 cached: 1 98304; id 50
c51 => s: 114688 diff: +16384 16% l 16 cached: 1 114688; id 51

c52 => s: 131072 diff: +16384 14% l 17 cached: 1 131072; id 52

c53 => s: 512 diff: +0 00% l 0 cached: 16 8192; id 20
```
I didn't hit any snag anywhere, all tests pass for check-asan, check-sanitizer, etc.

It's still fair game to loop on `kNumClasses` anywhere, as long as there is no assumption that `kNumClasses - 1` is the largest class.
As far as I can tell, no code anywhere is making that assumption.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_size_class_map.h:150
+    // sizeof(TransferBatch) where it matters.
+    if (UNLIKELY(class_id == kBatchClassID))
+      return kMaxNumCachedHint * sizeof(uptr);
----------------
alekseyshl wrote:
> Wouldn't these checks affect all other allocator's performance? From your earlier measurements, I recall that Size, ClassID and MaxCachedHint are pretty hot functions.
This is indeed the case, but we cached the hottest ones in the per-class arrays (in `class_size` and `max_count`), which is done on cache initialization (once per thread). The overhead of the other calls shouldn't be a problem.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_size_class_map.h:206
   static void Validate() {
-    for (uptr c = 1; c < kNumClasses; c++) {
+    for (uptr c = 1; c <= kLargestClassID; c++) {
       // Printf("Validate: c%zd\n", c);
----------------
alekseyshl wrote:
> Why this changed? Everywhere else iterating classes we use kNumClasses.
The issue is that `kBatchClassID` doesn't follow the same logic as the other class: its size is not larger than the one of the previous class, and it is meant to allocate only a single size. Meaning the test checking that size minus 1 still belongs to that class wouldn't pass.
In that sense, the validation code doesn't make sense for that particular class.


https://reviews.llvm.org/D37082