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

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added a comment.

Ok, overall, I have no better idea how to implement it, so let's iron out minor details and get it in.



================
Comment at: lib/sanitizer_common/sanitizer_allocator_local_cache.h:131
 struct SizeClassAllocator32LocalCache {
   typedef SizeClassAllocator Allocator;
   typedef typename Allocator::TransferBatch TransferBatch;
----------------
cryptoad wrote:
> 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?
Yes, if it is not too much of a burden, it would be great to make all non-API stuff private.


================
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);
----------------
cryptoad wrote:
> 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.
Maybe yes, it does not matter that much. Now I am more concerned with ClassID(ClassIdToSize(kBatchClassID)) != kBatchClassID, although I cannot point out where exactly it might be a problem.


================
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);
----------------
cryptoad wrote:
> 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.
It would be great if you could run that perf test with these changes, I'm still a bit concerned.


https://reviews.llvm.org/D37082