Background finalization.

Background finalization.

Flutter gallery app on Nexus 4:
 max weak handle processing (main thread): 17.7ms -> 1.1ms
 max scavenge (main thread): 21.0ms -> 17.1ms

BUG=flutter/flutter#3804
R=asiva@google.com

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

[rmacnak, 2016-06-02 19:22:40]

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

[rmacnak, 2016-06-02 19:22:41]

Trying to open a many subview in the Flutter gallery app,

longest time in weak handle processing
17.7 -> 1.1 ms cpu duration

longest time for a background finalization task
0 -> 22.8 ms cpu duration

longest time in a new-space gc
21.0 ms -> 17.1 ms cpu duration
(so clearly there are other causes of long new-space gcs)

[siva, 2016-06-02 20:18:05]

Could you also add in the CL description what your max scavenge times are
without this CL when running the same app.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl_test.cc:1224: ml.Wait();
Should this be WaitWithSafepointCheck(...) here?

We could have somebody using this in a test case that runs Dart code and has the
background compiler running.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_sta...
File runtime/vm/dart_api_state.h (right):

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_sta...
runtime/vm/dart_api_state.h:238: }
Maybe more readable as

void UpdateUnreachable(Isolate* isolate, FinalizationQueue* queue) {
  EnsureFreeExternal(isolate);
  if (queue != NULL) {
    MarkForFinalization();
    queue->Add(this);
  } else {
    Finalize(isolate, this);
  }
}

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:433:
handle->UpdateUnreachableFinalizeNow(thread()->isolate());
Why is this finalization done eagerly inline and not in the background like in
the scavenge case?

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:26: #include "vm/dart_api_state.h"
Why is 'dart_api_state.h' included here?

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:17: #include "vm/thread_pool.h"
Why is thread_pool.h included here?

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:28: class FinalizablePersistentHandle;
Is this forward declaration necessary

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:212: finalization_queue_->Add(handle);
handle->UpdateUnreachable(thread()->isolate(), finalization_queue);

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:220: MallocGrowableArray<FinalizablePersistentHandle*>*
finalization_queue_;
Maybe typedef MallocGrowableArray<FinalizablePersistentHandle*>
to FinalizationQueue.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:228: explicit BackgroundFinalizer(
Why explicit? seems to have more than 1 argument.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:258: // Exit isolate cleanly *before* notifying it, to
avoid shutdown race.
notifying it? (what is it).

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:265: ml.Notify();
seems odd that the thread has exited the isolate but is accessing the isolate's
heap.

Why is it necessary to exit the isolate before notifying that one finalization
task is done?

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:878: finalization_queue));
I am assuming the BackgroundFinalizer object created here is deleted by the
thread pool.

[rmacnak, 2016-06-03 01:07:33]

Description was changed from

==========
Background finalization.

BUG=
==========

to

==========
Background finalization.

Flutter gallery app:
 max weak handle processing (main thread): 17.7ms -> 1.1ms
 max scavenge (main thread): 21.0ms -> 17.1ms

BUG=flutter/flutter#3804
==========

[rmacnak, 2016-06-03 01:07:42]

Description was changed from

==========
Background finalization.

Flutter gallery app:
 max weak handle processing (main thread): 17.7ms -> 1.1ms
 max scavenge (main thread): 21.0ms -> 17.1ms

BUG=flutter/flutter#3804
==========

to

==========
Background finalization.

Flutter gallery app on Nexus 4:
 max weak handle processing (main thread): 17.7ms -> 1.1ms
 max scavenge (main thread): 21.0ms -> 17.1ms

BUG=flutter/flutter#3804
==========

[rmacnak, 2016-06-03 01:08:31]

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl_test.cc:1224: ml.Wait();
On 2016/06/02 20:18:05, siva wrote:
> Should this be WaitWithSafepointCheck(...) here?
> 
> We could have somebody using this in a test case that runs Dart code and has
the
> background compiler running.

Done.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_sta...
File runtime/vm/dart_api_state.h (right):

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/dart_api_sta...
runtime/vm/dart_api_state.h:238: }
On 2016/06/02 20:18:05, siva wrote:
> Maybe more readable as
> 
> void UpdateUnreachable(Isolate* isolate, FinalizationQueue* queue) {
>   EnsureFreeExternal(isolate);
>   if (queue != NULL) {
>     MarkForFinalization();
>     queue->Add(this);
>   } else {
>     Finalize(isolate, this);
>   }
> }

Yes, I like that better.

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:433:
handle->UpdateUnreachableFinalizeNow(thread()->isolate());
On 2016/06/02 20:18:05, siva wrote:
> Why is this finalization done eagerly inline and not in the background like in
> the scavenge case?

Added background finalization here too.

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:26: #include "vm/dart_api_state.h"
On 2016/06/02 20:18:05, siva wrote:
> Why is 'dart_api_state.h' included here?

I started with BackgroundFinalizer in this file. Removed.

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:17: #include "vm/thread_pool.h"
On 2016/06/02 20:18:05, siva wrote:
> Why is thread_pool.h included here?

Leftover, removed.

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

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:220: MallocGrowableArray<FinalizablePersistentHandle*>*
finalization_queue_;
On 2016/06/02 20:18:05, siva wrote:
> Maybe typedef MallocGrowableArray<FinalizablePersistentHandle*>
> to FinalizationQueue.

Done.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:228: explicit BackgroundFinalizer(
On 2016/06/02 20:18:05, siva wrote:
> Why explicit? seems to have more than 1 argument.
Removed.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:265: ml.Notify();
On 2016/06/02 20:18:05, siva wrote:
> seems odd that the thread has exited the isolate but is accessing the
isolate's
> heap.
> 
> Why is it necessary to exit the isolate before notifying that one finalization
> task is done?

This pattern comes from the sweeper task. This avoids running
ExitIsolateAsHelper after isolate shutdown.

https://codereview.chromium.org/2012973002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:878: finalization_queue));
On 2016/06/02 20:18:05, siva wrote:
> I am assuming the BackgroundFinalizer object created here is deleted by the
> thread pool.

Yes, tasks are deleted by the thread pool after they finish.

[siva, 2016-06-06 21:25:31]

LGTM with 2 comments.

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/dart_...
File runtime/vm/dart_api_state.h (right):

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/dart_...
runtime/vm/dart_api_state.h:578: return handle;
I am wondering if we should write this as :

{
  MutexLocker ml(mutex_);
  FinalizablePersistentHandle* handle;
  if (free_list_ != NULL) {
     ....
     ....
     ....
     return handle;
   }
}
handle = .....(AllocateScopedHandle());
handle->Clear();
return handle;

So that we are not holding on to the lock across the allocate scoped handle
stuff which could take longer.

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/heap....
runtime/vm/heap.cc:63: }
delete finalization_tasks_lock_;

[rmacnak, 2016-06-07 21:36:48]

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/dart_...
File runtime/vm/dart_api_state.h (right):

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/dart_...
runtime/vm/dart_api_state.h:578: return handle;
On 2016/06/06 21:25:31, siva wrote:
> I am wondering if we should write this as :
> 
> {
>   MutexLocker ml(mutex_);
>   FinalizablePersistentHandle* handle;
>   if (free_list_ != NULL) {
>      ....
>      ....
>      ....
>      return handle;
>    }
> }
> handle = .....(AllocateScopedHandle());
> handle->Clear();
> return handle;
> 
> So that we are not holding on to the lock across the allocate scoped handle
> stuff which could take longer.

Done.

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://chromiumcodereview.appspot.com/2012973002/diff/60001/runtime/vm/heap....
runtime/vm/heap.cc:63: }
On 2016/06/06 21:25:31, siva wrote:
> delete finalization_tasks_lock_;

Done.

[rmacnak, 2016-06-07 21:38:41]

Description was changed from

==========
Background finalization.

Flutter gallery app on Nexus 4:
 max weak handle processing (main thread): 17.7ms -> 1.1ms
 max scavenge (main thread): 21.0ms -> 17.1ms

BUG=flutter/flutter#3804
==========

to

==========
Background finalization.

Flutter gallery app on Nexus 4:
 max weak handle processing (main thread): 17.7ms -> 1.1ms
 max scavenge (main thread): 21.0ms -> 17.1ms

BUG=flutter/flutter#3804
R=asiva@google.com

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

[rmacnak, 2016-06-07 21:38:42]

Committed patchset #5 (id:80001) manually as
082829a00b012379dd86ebc77cd67dc1ef762532 (presubmit successful).