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

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_local_cache.h:131
 struct SizeClassAllocator32LocalCache {
   typedef SizeClassAllocator Allocator;
   typedef typename Allocator::TransferBatch TransferBatch;
----------------
Do we really need all these types and consts to be public?


================
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;
----------------
Add {} around in and else parts.


================
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);
----------------
Shouldn't it be also guarded by kUseSeparateSizeClassForBatch flag?


================
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;
----------------
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?


================
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);
----------------
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.


================
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);
----------------
Why this changed? Everywhere else iterating classes we use kNumClasses.


https://reviews.llvm.org/D37082