[PATCH] D28068: Whenever reasonable, merge ASAN quarantine batches to save memory.

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl created this revision.
alekseyshl added a reviewer: eugenis.
alekseyshl added a subscriber: llvm-commits.
Herald added a subscriber: kubabrecka.

There are cases when thread local quarantine drains almost empty
quarantine batches into the global quarantine. The current approach leaves
them almost empty, which might create a huge memory overhead (each batch
is 4K/8K, depends on bitness).


https://reviews.llvm.org/D28068

Files:
  lib/sanitizer_common/sanitizer_quarantine.h
  test/asan/TestCases/Linux/thread_local_quarantine_size_kb.cc


Index: test/asan/TestCases/Linux/thread_local_quarantine_size_kb.cc
===================================================================
--- test/asan/TestCases/Linux/thread_local_quarantine_size_kb.cc
+++ test/asan/TestCases/Linux/thread_local_quarantine_size_kb.cc
@@ -6,7 +6,7 @@
 // RUN: %env_asan_opts=thread_local_quarantine_size_kb=64:quarantine_size_mb=64 %run %t 2>&1 | \
 // RUN:   FileCheck %s --allow-empty --check-prefix=CHECK-SMALL-LOCAL-CACHE-SMALL-OVERHEAD
 // RUN: %env_asan_opts=thread_local_quarantine_size_kb=0:quarantine_size_mb=64 %run %t 2>&1 | \
-// RUN:   FileCheck %s --check-prefix=CHECK-NO-LOCAL-CACHE-HUGE-OVERHEAD
+// RUN:   FileCheck %s --check-prefix=CHECK-NO-LOCAL-CACHE-SMALL-OVERHEAD
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -17,6 +17,8 @@
 // total, and verify that quarantine does not incur too much memory overhead.
 // There's always an overhead for red zones, shadow memory and such, but
 // quarantine accounting should not significantly contribute to that.
+// The zero sized thread local cache is specifically tested since it used to
+// generate a huge overhead of almost empty quarantine batches.
 static const int kNumAllocs = 20000;
 static const int kAllocSize = 256;
 static const size_t kHeapSizeLimit = 12 << 20;
@@ -37,4 +39,4 @@
 
 // CHECK-VALUE: thread_local_quarantine_size_kb=256K
 // CHECK-SMALL-LOCAL-CACHE-SMALL-OVERHEAD-NOT: Heap size limit exceeded
-// CHECK-NO-LOCAL-CACHE-HUGE-OVERHEAD: Heap size limit exceeded
+// CHECK-NO-LOCAL-CACHE-SMALL-OVERHEAD-NOT: Heap size limit exceeded
Index: lib/sanitizer_common/sanitizer_quarantine.h
===================================================================
--- lib/sanitizer_common/sanitizer_quarantine.h
+++ lib/sanitizer_common/sanitizer_quarantine.h
@@ -65,10 +65,13 @@
   }
 
   void NOINLINE Drain(Cache *c, Callback cb) {
+    QuarantineBatch *batch_to_deallocate = 0;
     {
       SpinMutexLock l(&cache_mutex_);
-      cache_.Transfer(c);
+      batch_to_deallocate = cache_.Transfer(c);
     }
+    if (batch_to_deallocate)
+      cb.Deallocate(batch_to_deallocate);
     if (cache_.Size() > GetSize() && recycle_mutex_.TryLock())
       Recycle(cb);
   }
@@ -143,10 +146,36 @@
     SizeAdd(size);
   }
 
-  void Transfer(QuarantineCache *c) {
-    list_.append_back(&c->list_);
-    SizeAdd(c->Size());
-    atomic_store(&c->size_, 0, memory_order_relaxed);
+  QuarantineBatch *Transfer(QuarantineCache *from_cache) {
+    QuarantineBatch *batch_to_deallocate = 0;
+    if (list_.empty() || from_cache->list_.empty() ||
+        from_cache->list_.front()->next ||
+        from_cache->list_.front()->count + list_.back()->count >
+            QuarantineBatch::kSize) {
+      // Besides the trivial case of empty lists, there are either more than one
+      // batch to transfer or there're too many elements in the first batch and
+      // even if this cache's last batch is not full yet, it's too costly to
+      // move elements one by one, so we just accept memory penalty (the wasted
+      // space in this cache's last batch array).
+      // TODO(alekseyshl): consider transferring full batches and merge
+      // partially filled tail ones.
+      list_.append_back(&from_cache->list_);
+      SizeAdd(from_cache->Size());
+    } else {
+      // Two tail batches can be merged maintaining batch and cache invariants.
+      QuarantineBatch *from_batch = from_cache->DequeueBatch();
+      QuarantineBatch *to_batch = list_.back();
+      for (uptr i = 0; i < from_batch->count; ++i)
+        to_batch->batch[to_batch->count + i] = from_batch->batch[i];
+      to_batch->count += from_batch->count;
+      CHECK_GT(from_batch->size, sizeof(QuarantineBatch));
+      uptr transferred_nodes_size = from_batch->size - sizeof(QuarantineBatch);
+      to_batch->size += transferred_nodes_size;
+      SizeAdd(transferred_nodes_size);
+      batch_to_deallocate = from_batch;
+    }
+    atomic_store(&from_cache->size_, 0, memory_order_relaxed);
+    return batch_to_deallocate;
   }
 
   void EnqueueBatch(QuarantineBatch *b) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D28068.82374.patch
Type: text/x-patch
Size: 4059 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161222/ace66008/attachment.bin>