- Stop-the-world sweep uses pre-locked free list access.

runtime/vm/pages.cc

Index: runtime/vm/pages.cc
===================================================================
--- runtime/vm/pages.cc	(revision 39397)
+++ runtime/vm/pages.cc	(working copy)
@@ -565,38 +565,46 @@
   freelist_[HeapPage::kExecutable].Reset();
 
   int64_t mid2 = OS::GetCurrentTimeMicros();
+  int64_t mid3 = 0;
 
   GCSweeper sweeper(heap_);
 
-  HeapPage* prev_page = NULL;
-  HeapPage* page = pages_;
-  while (page != NULL) {
-    HeapPage* next_page = page->next();
-    bool page_in_use = sweeper.SweepPage(page, &freelist_[page->type()]);
-    if (page_in_use) {
-      prev_page = page;
-    } else {
-      FreePage(page, prev_page);
+  // During stop-the-world phases we should use bulk lock when adding elements
+  // to the free list.
+  {
+    MutexLocker mld(freelist_[HeapPage::kData].mutex());
+    MutexLocker mle(freelist_[HeapPage::kExecutable].mutex());
+
+    HeapPage* prev_page = NULL;
+    HeapPage* page = pages_;
+    while (page != NULL) {
+      HeapPage* next_page = page->next();
+      bool page_in_use = sweeper.SweepPage(page, &freelist_[page->type()]);
+      if (page_in_use) {
+        prev_page = page;
+      } else {
+        FreePage(page, prev_page);
+      }
+      // Advance to the next page.
+      page = next_page;
     }
-    // Advance to the next page.
-    page = next_page;
-  }
 
-  int64_t mid3 = OS::GetCurrentTimeMicros();
+    mid3 = OS::GetCurrentTimeMicros();
 
-  prev_page = NULL;
-  page = large_pages_;
-  while (page != NULL) {
-    HeapPage* next_page = page->next();
-    const intptr_t words_to_end = sweeper.SweepLargePage(page);
-    if (words_to_end == 0) {
-      FreeLargePage(page, prev_page);
-    } else {
-      TruncateLargePage(page, words_to_end << kWordSizeLog2);
-      prev_page = page;
+    prev_page = NULL;
+    page = large_pages_;
+    while (page != NULL) {
+      HeapPage* next_page = page->next();
+      const intptr_t words_to_end = sweeper.SweepLargePage(page);
+      if (words_to_end == 0) {
+        FreeLargePage(page, prev_page);
+      } else {
+        TruncateLargePage(page, words_to_end << kWordSizeLog2);
+        prev_page = page;
+      }
+      // Advance to the next page.
+      page = next_page;
     }
-    // Advance to the next page.
-    page = next_page;
   }

[koda, 2014/08/20 13:52:22]

For a large scope like this, it might help readability to comment that locks are
released here.

[Ivan Posva, 2014/08/20 14:10:20]

In my experience commenting here is counter-productive. The whole point about
these stack objects is that they are automatically released and that you reduce
clutter in your code. If you have returns/loop exits in the middle of these
blocks then the comment is also incorrect. Unlocking will not block, so it is
also unnecessary from a performance/timing perspective.

[koda, 2014/08/20 16:11:19]

In any case, keeping method bodies within a reasonable size is a way to minimize
the issue. I think this one could benefit from some abstraction in a future CL.

 
   // Make code pages read-only.