[vm, gc] Require a safepoint for heap iteration.

[vm, gc] Require a safepoint for heap iteration.

Assert a safepoint operation can only be started from a thread in the VM state. Fix several unit tests missing a native-to-vm transition.

R=asiva@google.com, danunez@google.com

Committed: https://github.com/dart-lang/sdk/commit/466385ceb6cb03835d5e860e6aeff689e62fd5a0

[rmacnak, 2017-08-04 23:39:23]

rmacnak@google.com changed reviewers:
+ asiva@google.com, danunez@google.com

[danunez, 2017-08-05 17:24:12]

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/clustered_sn...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/clustered_sn...
runtime/vm/clustered_snapshot.cc:5355:
iteration.IterateObjects(&token_streams_counter);
It looks like this version will collect token streams from the new space, since
IterateObjects walks over both spaces. Is this change intended (a fix from the
previous version)?

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

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:248: heap_->old_space()->VisitObjectsNoImagePages(visitor);
I would just use old_space_ since we have initialized it in the constructor.

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/object_graph...
File runtime/vm/object_graph_test.cc (right):

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph_test.cc:60: RawObject* b_raw = b.raw();
I think we can remove the NoSafepoint here because our mutator thread is not
going to be allocating and we shouldn't have a background thread doing any real
work in this test.

Am I correct in that?

If there is a background thread doing allocation in this test, we could see this
test come up later when background threads start allocating alongside mutators.

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (right):

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:136: iteration.IterateStackPointers(&counter,
Why did we change to iterating over just the stack pointers?

The original function also visited class tables and object stores. Are those
visits not necessary for the test?

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:153: iteration.IterateStackPointers(&str_counter,
Ditto.

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:436: iteration.IterateStackPointers(&counter,
Ditto.

[rmacnak, 2017-08-07 22:37:51]

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/clustered_sn...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/clustered_sn...
runtime/vm/clustered_snapshot.cc:5355:
iteration.IterateObjects(&token_streams_counter);
On 2017/08/05 17:24:11, danunez wrote:
> It looks like this version will collect token streams from the new space,
since
> IterateObjects walks over both spaces. Is this change intended (a fix from the
> previous version)?

Token streams are allocated into old space, which is why the old code works. The
change is mostly to keep symmetry between iterating the two isolates will
avoiding introducing extra every more variants of iteration, e.g.,
IterateVMIsolateOldObjects.

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/object_graph...
File runtime/vm/object_graph_test.cc (right):

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph_test.cc:60: RawObject* b_raw = b.raw();
On 2017/08/05 17:24:11, danunez wrote:
> I think we can remove the NoSafepoint here because our mutator thread is not
> going to be allocating and we shouldn't have a background thread doing any
real
> work in this test.
> 
> Am I correct in that?
> 
> If there is a background thread doing allocation in this test, we could see
this
> test come up later when background threads start allocating alongside
mutators.

Nevermind, this is unsafe in the unlikely case that the test harness has the
background compiler running. Replaced with a SafepointOperationScope.

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (right):

https://codereview.chromium.org/2995543004/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:136: iteration.IterateStackPointers(&counter,
On 2017/08/05 17:24:11, danunez wrote:
> Why did we change to iterating over just the stack pointers?
> 
> The original function also visited class tables and object stores. Are those
> visits not necessary for the test?

The test only needs to visit zone handles.

[danunez, 2017-08-07 22:57:47]

lgtm

[siva, 2017-08-07 23:28:38]

lgtm

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

https://codereview.chromium.org/2995543004/diff/40001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:370: explicit HeapIterationScope(bool writable = false);
Why not add the 'Thread* thread' parameter here.

[rmacnak, 2017-08-08 00:29:41]

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

https://codereview.chromium.org/2995543004/diff/40001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:370: explicit HeapIterationScope(bool writable = false);
On 2017/08/07 23:28:38, siva wrote:
> Why not add the 'Thread* thread' parameter here.

Done.

[rmacnak, 2017-08-08 00:51:30]

Description was changed from

==========
[vm, gc] Require a safepoint for heap iteration.

Assert a safepoint operation can only be started from a thread in the VM state.
Fix several unit tests missing a native-to-vm transition.
==========

to

==========
[vm, gc] Require a safepoint for heap iteration.

Assert a safepoint operation can only be started from a thread in the VM state.
Fix several unit tests missing a native-to-vm transition.

R=asiva@google.com, danunez@google.com

Committed:
https://github.com/dart-lang/sdk/commit/466385ceb6cb03835d5e860e6aeff689e62fd5a0
==========

[rmacnak, 2017-08-08 00:51:31]

Committed patchset #4 (id:60001) manually as
466385ceb6cb03835d5e860e6aeff689e62fd5a0 (presubmit successful).