[PATCH] D46657: [sanitizer] Minor 32-bit primary improvements

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_local_cache.h:192
       while (c->count > 0)
-        Drain(c, allocator, i);
+        Drain(c, allocator, i, Min(c->max_count / 2, c->count));
     }
----------------
cryptoad wrote:
> alekseyshl wrote:
> > Why the loop, why don't we drain the class in one call?
> > 
> >   Drain(c, allocator, i, c->count);
> > 
> > We used to have to have a loop because of the Drain API, but now we can specify the number of elements to drain.
> Since the `TransferBatch` that we are using can only hold `max_count / 2`, we can't directly drain `count`.
> I could change it to something like  if `count > max_count / 2` then we can drain `max_count / 2`, then drain `count` elements.
Ah, I see.

This restriction was not obvious before and it is even more obfuscated now. Was Min() call in Drain so taxing that it is worth complicating the already not so easy to follow logic of the code? If it is not, I'd rather not change the code. If it is though, well, do it, but add a DCHECK to Drain to document the restriction of this API:  DCHECK_LE(count, c->max_count / 2); and ignore my previous comment, Drain(c, allocator, class_id, c->max_count / 2);


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46657