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

runtime/vm/pages.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/pages.h"
 
 #include "platform/assert.h"
 #include "vm/compiler_stats.h"
 #include "vm/gc_marker.h"
 #include "vm/gc_sweeper.h"
@@ skipping 547 matching lines @@
   usage_.used_in_words = marker.marked_words();
 
   int64_t mid1 = OS::GetCurrentTimeMicros();
 
   // Reset the bump allocation page to unused.
   // Reset the freelists and setup sweeping.
   freelist_[HeapPage::kData].Reset();
   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.
   WriteProtectCode(true);
 
   int64_t end = OS::GetCurrentTimeMicros();
 
   // Record signals for growth control. Include size of external allocations.
   page_space_controller_.EvaluateGarbageCollection(usage_before, usage_,
                                                    start, end);
 
@@ skipping 138 matching lines @@
     return 0;
   } else {
     ASSERT(total_time >= gc_time);
     int result= static_cast<int>((static_cast<double>(gc_time) /
                              static_cast<double>(total_time)) * 100);
     return result;
   }
 }
 
 }  // namespace dart