[PATCH] D34152: [sanitizer] MmapAlignedOrDie changes to reduce fragmentation

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_common.h:98
+// some unmapping operations and reduce memory fragmentation. That chunk will
+// hold 'alignment' bytes.
+void *MmapAlignedOrDie(uptr size, uptr alignment, const char *mem_type,
----------------
How about this:

// Since the predominant use case of this function is "size == alignment" and the nature of the
// way the alignment requirement is satisfied (by allocating size+alignment bytes of memory), there's a potential
// of address space fragmentation. The padding_chunk parameter provides the opportunity
// to return the contiguous padding of "size" bytes of the allocated chunk if the initial allocation happened to be
// perfectly aligned and the platform supports partial unmapping of the mapped region.



================
Comment at: lib/sanitizer_common/sanitizer_posix.cc:159
+  bool is_aligned = IsAligned(map_res, alignment);
+  if (is_aligned && padding_chunk) {
+    *padding_chunk = map_res + size;
----------------
Just realized that it should be 

if (is_aligned && padding_chunk && size == alignment) {
   ...

The 'second chunk' logic does not work when size != alignment. With some improvements, it might help with size < alignment case, but, fortunately, our only use case for now is size == alignment, let's not complicate it more than necessary.


================
Comment at: lib/sanitizer_common/tests/sanitizer_common_test.cc:130
+        }
+      }
+    }
----------------
Since the structure of the test is the same, maybe we should move #if#else#endinf into the loop?I mean, ifdef'ing only lines 100-103 and 118-129?


        EXPECT_EQ(0U, res % (alignment * PageSize));
        internal_memset((void*)res, 1, size * PageSize);
  #if SANITIZER_WINDOWS
        // padding_chunk is not supported on Windows.
        EXPECT_EQ(0U, padding_chunk);
        UnmapOrDie((void*)res, size * PageSize);
  #else
        if (size == 1 && alignment == 1) {
          // mmap returns PageSize aligned chunks, so this is a specific case
          // where we can check that padding_chunk will never be 0.
          EXPECT_NE(0U, padding_chunk);
        }
        if (padding_chunk) {
          EXPECT_EQ(res + size * PageSize, padding_chunk);
          internal_memset((void*)padding_chunk, 1, alignment * PageSize);
          UnmapOrDie((void*)padding_chunk, alignment * PageSize);
        }
  #endif


https://reviews.llvm.org/D34152