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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 2 inline comments as done.
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:
> 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.
Working on it.


================
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:
> 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.
It does show in the `SizeClassMap::Print`:
```
c53 => s: 512 diff: +0 00% l 0 cached: 16 8192; id 20
```
53 is the `kBatchClassID`,  and the `ClassID(ClassIdToSize(kBatchClassID))` is 20.
I didn't think much of it, it could be solved with a special case in `ClassID`.


================
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:
> 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.
Will do.


================
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);
----------------
cryptoad wrote:
> 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.
Revisiting this: I can loop on `kNumClasses` and make a special case for `kBatchClassID`. Validate is only a test/debug functionality anyway.


https://reviews.llvm.org/D37082