Changed the PageIterator class so that it only returns pages existing...

src/spaces.h

Index: src/spaces.h
===================================================================
--- src/spaces.h	(revision 1888)
+++ src/spaces.h	(working copy)
@@ -511,11 +511,22 @@
 //
 // 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 @@
 
 
 // -----------------------------------------------------------------------------
-// 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
+  };

[Erik Corry, 2009/05/07 09:48:07]

I liked the old version better.  Does the style guide mandate this?

[Kevin Millikin (Chromium), 2009/05/07 10:17:17]

Not mandated and we have a mix in the codebase.  I've been migrating to this....

 
   PageIterator(PagedSpace* space, Mode mode);
 
@@ -577,8 +606,9 @@
   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,9 @@
   // The first page in this space.
   Page* first_page_;
 
+  // The last page in this space.
+  Page* last_page_;

[Erik Corry, 2009/05/07 09:48:07]

The way this is updated is subtle.  It at least deserves a comment here.

[Kevin Millikin (Chromium), 2009/05/07 10:17:17]

Commented here and in the places where it's modified.

+
   // Normal allocation information.
   AllocationInfo allocation_info_;