Safer interface for heap iteration.

Safer interface for heap iteration.

Address issue 21620, by sharing code to wait for concurrent sweeper to finish, and freeing the caller from this responsibility. The new public interface is Heap::Iterate*, and the old Heap::Visit* are made private, for GC use only.

(Eventually, we'll want to support iterating over the heap while concurrent GC runs, but that will have to wait until all header accesss is synchronized.)

BUG=21620
R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/3fc6fce1c03328f4201bacfd9f8f6de0cef88376

[koda, 2015-07-07 04:40:39]

koda@google.com changed reviewers:
+ asiva@google.com

[siva, 2015-07-07 18:10:24]

I have one comment regarding nested HeapIterationScopes.

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.cc#newcode231
runtime/vm/heap.cc:231: old_space_->set_tasks(1);
How do we ensure that people don't end up with nested HeapIterationScopes? Maybe
we need an assertion to prevent that.

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.cc#newcode236
runtime/vm/heap.cc:236: old_space_->set_tasks(0);
Doesn't this need to be set under a lock as well?

MonitorLocker ml(old_space_->tasks_lock());
ASSERT(old_space_->tasks() == 1);
old_space_->set_tasks(0);
ml.notify();

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.h
File runtime/vm/heap.h (right):

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.h#newcode158
runtime/vm/heap.h:158: bool VerifyGC(MarkExpectation mark_expectation =
kForbidMarked) const;
Can this be a private function?

[koda, 2015-07-07 22:46:40]

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.cc#newcode231
runtime/vm/heap.cc:231: old_space_->set_tasks(1);
On 2015/07/07 18:10:24, siva wrote:
> How do we ensure that people don't end up with nested HeapIterationScopes?
Maybe
> we need an assertion to prevent that.

Done.

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.cc#newcode236
runtime/vm/heap.cc:236: old_space_->set_tasks(0);
On 2015/07/07 18:10:24, siva wrote:
> Doesn't this need to be set under a lock as well?
> 
> MonitorLocker ml(old_space_->tasks_lock());
> ASSERT(old_space_->tasks() == 1);
> old_space_->set_tasks(0);
> ml.notify();

Done.

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.h
File runtime/vm/heap.h (right):

https://codereview.chromium.org/1212943010/diff/1/runtime/vm/heap.h#newcode158
runtime/vm/heap.h:158: bool VerifyGC(MarkExpectation mark_expectation =
kForbidMarked) const;
On 2015/07/07 18:10:24, siva wrote:
> Can this be a private function?

Done.

[siva, 2015-07-07 22:53:42]

lgtm

https://codereview.chromium.org/1212943010/diff/40001/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/1212943010/diff/40001/runtime/vm/pages.cc#new...
runtime/vm/pages.cc:162: tasks_(0),
#if defined(DEBUG)

https://codereview.chromium.org/1212943010/diff/40001/runtime/vm/pages.cc#new...
runtime/vm/pages.cc:163: is_iterating_(false),
#endif

[koda, 2015-07-08 00:18:56]

On 2015/07/07 22:53:42, siva wrote:
> lgtm
> 
> https://codereview.chromium.org/1212943010/diff/40001/runtime/vm/pages.cc
> File runtime/vm/pages.cc (right):
> 
>
https://codereview.chromium.org/1212943010/diff/40001/runtime/vm/pages.cc#new...
> runtime/vm/pages.cc:162: tasks_(0),
> #if defined(DEBUG)
> 
>
https://codereview.chromium.org/1212943010/diff/40001/runtime/vm/pages.cc#new...
> runtime/vm/pages.cc:163: is_iterating_(false),
> #endif

Done.

[koda, 2015-07-08 00:20:25]

Committed patchset #4 (id:60001) manually as
3fc6fce1c03328f4201bacfd9f8f6de0cef88376 (presubmit successful).