Changed the PageIterator class so that it only returns pages existing

at construction time.  If allocation during iteration causes a paged
space to expand, the iterator will not return the new pages.

This makes it more closely match the HeapObjectIterator behavior, and
it removes a possible source of bugs (if the allocation top was in the
last page in the space, the old iterator would stop only when it
reached the end of the space, potentially returning invalid pages from
a freshly expanded space).

Review URL: http://codereview.chromium.org/115074

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1895 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/spaces-inl.h b/src/spaces-inl.h
index d7cddb4..3973658 100644 (file)
--- a/src/spaces-inl.h
+++ b/src/spaces-inl.h
@@ -64,15 +64,16 @@ HeapObject* HeapObjectIterator::next() {
 // PageIterator
 
 bool PageIterator::has_next() {
-  return cur_page_ != stop_page_;
+  return prev_page_ != stop_page_;
 }
 
 
 Page* PageIterator::next() {
   ASSERT(has_next());
-  Page* result = cur_page_;
-  cur_page_ = cur_page_->next_page();
-  return result;
+  prev_page_ = (prev_page_ == NULL)
+               ? space_->first_page_
+               : prev_page_->next_page();
+  return prev_page_;
 }
 
 

diff --git a/src/spaces.cc b/src/spaces.cc
index c124af1..f15af9e 100644 (file)
--- a/src/spaces.cc
+++ b/src/spaces.cc
@@ -111,17 +111,17 @@ void HeapObjectIterator::Verify() {
 // -----------------------------------------------------------------------------
 // PageIterator
 
-PageIterator::PageIterator(PagedSpace* space, Mode mode) {
-  cur_page_ = space->first_page_;
+PageIterator::PageIterator(PagedSpace* space, Mode mode) : space_(space) {
+  prev_page_ = NULL;
   switch (mode) {
     case PAGES_IN_USE:
-      stop_page_ = space->AllocationTopPage()->next_page();
+      stop_page_ = space->AllocationTopPage();
       break;
     case PAGES_USED_BY_MC:
-      stop_page_ = space->MCRelocationTopPage()->next_page();
+      stop_page_ = space->MCRelocationTopPage();
       break;
     case ALL_PAGES:
-      stop_page_ = Page::FromAddress(NULL);
+      stop_page_ = space->last_page_;
       break;
     default:
       UNREACHABLE();
@@ -496,8 +496,11 @@ bool PagedSpace::Setup(Address start, size_t size) {
   accounting_stats_.ExpandSpace(num_pages * Page::kObjectAreaSize);
   ASSERT(Capacity() <= max_capacity_);
 
+  // Sequentially initialize remembered sets in the newly allocated
+  // pages and cache the current last page in the space.
   for (Page* p = first_page_; p->is_valid(); p = p->next_page()) {
     p->ClearRSet();
+    last_page_ = p;
   }
 
   // Use first_page_ for allocation.
@@ -676,9 +679,11 @@ bool PagedSpace::Expand(Page* last_page) {
 
   MemoryAllocator::SetNextPage(last_page, p);
 
-  // Clear remembered set of new pages.
+  // Sequentially clear remembered set of new pages and and cache the
+  // new last page in the space.
   while (p->is_valid()) {
     p->ClearRSet();
+    last_page_ = p;
     p = p->next_page();
   }
 
@@ -723,10 +728,12 @@ void PagedSpace::Shrink() {
   Page* p = MemoryAllocator::FreePages(last_page_to_keep->next_page());
   MemoryAllocator::SetNextPage(last_page_to_keep, p);
 
-  // Since pages are only freed in whole chunks, we may have kept more than
-  // pages_to_keep.
+  // Since pages are only freed in whole chunks, we may have kept more
+  // than pages_to_keep.  Count the extra pages and cache the new last
+  // page in the space.
   while (p->is_valid()) {
     pages_to_keep++;
+    last_page_ = p;
     p = p->next_page();
   }
 

diff --git a/src/spaces.h b/src/spaces.h
index cdb7d29..f3ad81b 100644 (file)
--- a/src/spaces.h
+++ b/src/spaces.h
@@ -511,11 +511,22 @@ class ObjectIterator : public Malloced {
 //
 // A HeapObjectIterator iterates objects from a given address to the
 // top of a space. The given address must be below the current
-// allocation pointer (space top). If the space top changes during
-// iteration (because of allocating new objects), the iterator does
-// not iterate new objects. The caller function must create a new
-// iterator starting from the old top in order to visit these new
-// objects. Heap::Scavenage() is such an example.
+// allocation pointer (space top). There are some caveats.
+//
+// (1) If the space top changes upward during iteration (because of
+//     allocating new objects), the iterator does not iterate objects
+//     above the original space top. The caller must create a new
+//     iterator starting from the old top in order to visit these new
+//     objects. Heap::Scavenge() is such an example.
+//
+// (2) If new objects are allocated below the original allocation top
+//     (e.g., free-list allocation in paged spaces), the new objects
+//     may or may not be iterated depending on their position with
+//     respect to the current point of iteration.
+//
+// (3) The space top should not change downward during iteration,
+//     otherwise the iterator will return not-necessarily-valid
+//     objects.
 
 class HeapObjectIterator: public ObjectIterator {
  public:
@@ -559,17 +570,35 @@ class HeapObjectIterator: public ObjectIterator {
 
 
 // -----------------------------------------------------------------------------
-// A PageIterator iterates pages in a space.
+// A PageIterator iterates the pages in a paged space.
 //
 // The PageIterator class provides three modes for iterating pages in a space:
-//   PAGES_IN_USE iterates pages that are in use by the allocator;
-//   PAGES_USED_BY_GC iterates pages that hold relocated objects during a
-//                    mark-compact collection;
+//   PAGES_IN_USE iterates pages containing allocated objects.
+//   PAGES_USED_BY_MC iterates pages that hold relocated objects during a
+//                    mark-compact collection.
 //   ALL_PAGES iterates all pages in the space.
+//
+// There are some caveats.
+//
+// (1) If the space expands during iteration, new pages will not be
+//     returned by the iterator in any mode.
+//
+// (2) If new objects are allocated during iteration, they will appear
+//     in pages returned by the iterator.  Allocation may cause the
+//     allocation pointer or MC allocation pointer in the last page to
+//     change between constructing the iterator and iterating the last
+//     page.
+//
+// (3) The space should not shrink during iteration, otherwise the
+//     iterator will return deallocated pages.
 
 class PageIterator BASE_EMBEDDED {
  public:
-  enum Mode {PAGES_IN_USE, PAGES_USED_BY_MC, ALL_PAGES};
+  enum Mode {
+    PAGES_IN_USE,
+    PAGES_USED_BY_MC,
+    ALL_PAGES
+  };
 
   PageIterator(PagedSpace* space, Mode mode);
 
@@ -577,8 +606,9 @@ class PageIterator BASE_EMBEDDED {
   inline Page* next();
 
  private:
-  Page* cur_page_;  // next page to return
-  Page* stop_page_;  // page where to stop
+  PagedSpace* space_;
+  Page* prev_page_;  // Previous page returned.
+  Page* stop_page_;  // Page to stop at (last page returned by the iterator).
 };
 
 
@@ -809,6 +839,10 @@ class PagedSpace : public Space {
   // The first page in this space.
   Page* first_page_;
 
+  // The last page in this space.  Initially set in Setup, updated in
+  // Expand and Shrink.
+  Page* last_page_;
+
   // Normal allocation information.
   AllocationInfo allocation_info_;