[PATCH] D83993: [scudo][standalone] Change the release loop for efficiency purposes

[Peter Collingbourne via Phabricator via llvm-commits]

pcc accepted this revision.
pcc added a comment.

LGTM



================
Comment at: compiler-rt/lib/scudo/standalone/release.h:270
+            PInRegion += BlockSize;
+            while (PInRegion < RoundedRegionSize) {
+              Counters.incRange(RegionIndex, PInRegion >> PageSizeLog,
----------------
cryptoad wrote:
> cryptoad wrote:
> > pcc wrote:
> > > I guess this is equivalent to the code previously on lines 252-253. But I'm confused about why we would have more than one of these. In my mind these blocks would consume as much of the region as possible, which means that at the end we would just have a fraction of a block at most. Is that not the case?
> > I went for something generic, and I think you are right in the sense that the loop will only iterate once. I will verify it and update it if it's the case.
> I ran some tests, and I was wrong. There cases where there is more than 1 block.
> This is because we use AllocatedUser as the region size, which might not be a multiple of a page (it's bound by a maximum # of batches we want to allocate at a time), so there are several circumstances where there would be more than one block in the last page. Here are some examples that got logged on Android:
> block size, last block, region size, rounded region size
> 320, 173e0, 17700, 18000
> 1c0, 5e800, 5e9c0, 5f000
> 120, baf60, bb080, bc000
> 
> So the loop ends up necessary.
I see. This is fine then, but I wonder whether there's some way that we can avoid wasting space at the end of the region like this. This is follow-up territory of course though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83993/new/

https://reviews.llvm.org/D83993