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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad 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));
     }
----------------
alekseyshl wrote:
> 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);
Yeah you are right it's not worth it. I'll revert this part to its original state.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46657