Treat background finalization as another GC task so it won't run in parallel with the marker.

Treat background finalization as another GC task so it won't run in parallel with the marker.

Prevents a race between the marker and finalizer when the handle is freed (fixes #27029).

Disallow creating weak handles for immediate objects. We don't distinguish these from free handles, so their callbacks are never run.

Don't report freed handles over the vm service.

R=asiva@google.com

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

[rmacnak, 2016-08-26 19:31:08]

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

[siva, 2016-08-29 22:05:09]

LGTM with some comments.

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_impl_te...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_impl_te...
runtime/vm/dart_api_impl_test.cc:2427: OS::Sleep(10);
Why is this sleep necessary as you now make sure only one set of finalizer is
running?

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

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_state.c...
runtime/vm/dart_api_state.cc:24: isolate->heap()->old_space()->tasks() + 1);
PageSpace* old_space = heap_->old_space();
MonitorLocker ml(old_space->tasks_lock());
ols_space->set_tasks(old_space->tasks() + 1);

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_state.c...
runtime/vm/dart_api_state.cc:51:
heap->old_space()->set_tasks(heap->old_space()->tasks() - 1);
Ditto comment.

[rmacnak, 2016-08-29 23:09:45]

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_impl_te...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_impl_te...
runtime/vm/dart_api_impl_test.cc:2427: OS::Sleep(10);
On 2016/08/29 22:05:09, siva wrote:
> Why is this sleep necessary as you now make sure only one set of finalizer is
> running?

Trying to stress the situation where the marker has to wait.

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

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_state.c...
runtime/vm/dart_api_state.cc:24: isolate->heap()->old_space()->tasks() + 1);
On 2016/08/29 22:05:09, siva wrote:
> PageSpace* old_space = heap_->old_space();
> MonitorLocker ml(old_space->tasks_lock());
> ols_space->set_tasks(old_space->tasks() + 1);

Done.

https://codereview.chromium.org/2281193002/diff/1/runtime/vm/dart_api_state.c...
runtime/vm/dart_api_state.cc:51:
heap->old_space()->set_tasks(heap->old_space()->tasks() - 1);
On 2016/08/29 22:05:09, siva wrote:
> Ditto comment.

Done.

[rmacnak, 2016-08-29 23:48:00]

Description was changed from

==========
Treat background finalization as another GC task so it won't run in parallel
with the marker.

Prevents a race between the marker and finalizer when the handle is freed (fixes
#27029).

Disallow creating weak handles for immediate objects. We don't distinguish these
from free handles, so their callbacks are never run.

Don't report freed handles over the vm service.
==========

to

==========
Treat background finalization as another GC task so it won't run in parallel
with the marker.

Prevents a race between the marker and finalizer when the handle is freed (fixes
#27029).

Disallow creating weak handles for immediate objects. We don't distinguish these
from free handles, so their callbacks are never run.

Don't report freed handles over the vm service.

R=asiva@google.com

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

[rmacnak, 2016-08-29 23:48:01]

Committed patchset #2 (id:20001) manually as
e46b3cd99471fef0f6fadc464e66d000f38d70ea (presubmit successful).