[PATCH] D33454: [sanitizer] Change the 32-bit Primary AllocateRegion to reduce fragmentation

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl accepted this revision.
alekseyshl added a comment.
This revision is now accepted and ready to land.

LGTM with some code structure suggestions which you're free to ignore, if you like your way better.



================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:287
+    uptr map_res = (uptr)MmapOrDie(map_size, "SizeClassAllocator32");
+    uptr map_end = map_res + map_size;
+    uptr res = map_res;
----------------
I'd just inline map_res + map_size in the only place you use map_end.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:288
+    uptr map_end = map_res + map_size;
+    uptr res = map_res;
+    bool extra_region = false;
----------------
Can we rename it to 'region'?


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:289
+    uptr res = map_res;
+    bool extra_region = false;
+    if (IsAligned(res, kRegionSize)) {
----------------
The name 'extra_region' is a bit narrower than what this flag indicates. How about changing the structure a bit:

   bool trim_region = true;
   if (IsAligned(res, kRegionSize)) {
      SpinMutexLock l(&regions_stash_mutex);
      if (num_stashed_regions < kMaxStashedRegions) {
        ...
        trim_region = false;
      }
   }
   if (trim_region) {
     trim both left and right
   }

UnmapOrDie handles 0 sizes just fine.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:306
+      UnmapOrDie((void*)end, map_end - end);
+      map_size -= kRegionSize;
+    }
----------------
I know, the result is the same, but doesn't it make more sense to just assign here? It aligns better with how 'end' is calculated.

   map_size = kRegionSize;


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:358
 
+  static const uptr kMaxStashedRegions = 8;
+  SpinMutex regions_stash_mutex;
----------------
Please add your "unless several threads request regions simultaneously from different size classes, the stash rarely goes above 1 entry" comment to this constant.


https://reviews.llvm.org/D33454