Track external allocated memory for weak persistent handles.

Track external allocated memory for weak persistent handles.

The amount of external allocation is tracked per generation
and informs GC policy.

BUG=dart:11471
R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=33467

[koda, 2014-03-05 16:17:51]

I suspect there are more clients that will need to be updated with the new API
parameter. If you point me to them, I will update them.

[Ivan Posva, 2014-03-05 21:21:23]

First set of comments.

-Ivan

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/dart_api_state.h
File runtime/vm/dart_api_state.h (right):

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:202: void InitializeExternalAllocationSize(intptr_t
size, Heap* heap) {
Somehow this name does not feel right. As it is called in a sequence of set_XYZ.

Maybe it should just be SetExternalSize() because once the size is set it is not
going to change like the other fields of the handle

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:239: external_allocation_size_(0),
How about just external_size?

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

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:107: void AllocateExternal(intptr_t size, RawObject* ref);
Do we allow non-heap objects (aka Smis) do be used as reference in weak handles?

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:173: intptr_t ExternalAllocatedInWords(Space space) const;
How about ExternalInWords?

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

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:561: used_in_words += external_allocated_in_words_;
We probably want to track these separately in the end. For sure we want to print
them separatedly.

[koda, 2014-03-05 21:52:08]

PTAL

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/dart_api_state.h
File runtime/vm/dart_api_state.h (right):

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:202: void InitializeExternalAllocationSize(intptr_t
size, Heap* heap) {
On 2014/03/05 21:21:23, Ivan Posva wrote:
> Somehow this name does not feel right. As it is called in a sequence of
set_XYZ.
> 
> Maybe it should just be SetExternalSize() because once the size is set it is
not
> going to change like the other fields of the handle

Done.

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:239: external_allocation_size_(0),
On 2014/03/05 21:21:23, Ivan Posva wrote:
> How about just external_size?

Done.

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

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:107: void AllocateExternal(intptr_t size, RawObject* ref);
On 2014/03/05 21:21:23, Ivan Posva wrote:
> Do we allow non-heap objects (aka Smis) do be used as reference in weak
handles?

Yes, I believe we do. See my test
WeakPersistentHandleExternalAllocationSizeOddReferents.

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:173: intptr_t ExternalAllocatedInWords(Space space) const;
On 2014/03/05 21:21:23, Ivan Posva wrote:
> How about ExternalInWords?

Done.

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

https://codereview.chromium.org/187113003/diff/30013/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:561: used_in_words += external_allocated_in_words_;
On 2014/03/05 21:21:23, Ivan Posva wrote:
> We probably want to track these separately in the end. For sure we want to
print
> them separatedly.

Done.

[siva, 2014-03-06 17:17:39]

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2137: weak1 = Dart_NewWeakPersistentHandle(obj,
NULL, 1024, NULL);
could we have constants
const intptr_t kWeak1ExternalSize = 1 * KB;
const intptr_t kWeak2ExternalSize = 2 * KB;
and use these in the tests instead of 1024, 2048 etc.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2161: Dart_DeletePersistentHandle(strong_ref);
Should we invoke the garbage collector here and check after the weak persistent
handle has been deleted as to whether heap->ExternalInWords(...) is back to 0?

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2172: weak1 = Dart_NewWeakPersistentHandle(obj,
NULL, 100*MB, NULL);
ditto comment here about use of constants.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2190: Dart_DeleteWeakPersistentHandle(weak1);
Ditto comment regarding check.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2202: weak1 =
Dart_NewWeakPersistentHandle(dart_null, NULL, 1024, NULL);
Ditto.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2213: Dart_DeleteWeakPersistentHandle(weak2);
Ditto comment regarding check.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_state.h
File runtime/vm/dart_api_state.h (right):

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:266: intptr_t external_size_;
Not for this CL but just a thought:
Now that we have an external field I am wondering if we should steal a bit from
here to indicate if the handle is a prolog weak persistent handle or a regular
weak persistent handle (current we tag the returned pointer).

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

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:108: void FreeExternal(intptr_t size, RawObject* ref);
Normally we try to avoid passing in raw pointers as parameters to functions
unless they are special functions that are invoked during GC or snapshot
writing. Why not change this API to

void AllocateExternal(intptr_t size, Space space);
void FreeExternal(intptr_t size, Space space);

[Ivan Posva, 2014-03-06 21:59:33]

-Ivan

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_state.h
File runtime/vm/dart_api_state.h (right):

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:202: void SetExternalAllocationSize(intptr_t size,
Heap* heap) {
SetExternalSize() to match the field.

[koda, 2014-03-06 22:55:07]

Updated and merged.

Also added a clarification to the API comment regarding the semantics of early
deletion: the callback is not run.

The implementation considers the external allocation freed when the referent
becomes unreachable or the handle is explicitly deleted, whatever happens first.

Non-heap and VM-heap objects count as old space for external size accounting
purposes. (This should be rare, but if it does happen, it seemed most consistent
to keep the best picture of total memory pressure.)

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2137: weak1 = Dart_NewWeakPersistentHandle(obj,
NULL, 1024, NULL);
On 2014/03/06 17:17:39, siva wrote:
> could we have constants
> const intptr_t kWeak1ExternalSize = 1 * KB;
> const intptr_t kWeak2ExternalSize = 2 * KB;
> and use these in the tests instead of 1024, 2048 etc.

Done.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2161: Dart_DeletePersistentHandle(strong_ref);
On 2014/03/06 17:17:39, siva wrote:
> Should we invoke the garbage collector here and check after the weak
persistent
> handle has been deleted as to whether heap->ExternalInWords(...) is back to 0?

Done.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2172: weak1 = Dart_NewWeakPersistentHandle(obj,
NULL, 100*MB, NULL);
On 2014/03/06 17:17:39, siva wrote:
> ditto comment here about use of constants.

Done.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2190: Dart_DeleteWeakPersistentHandle(weak1);
On 2014/03/06 17:17:39, siva wrote:
> Ditto comment regarding check.

Done.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2202: weak1 =
Dart_NewWeakPersistentHandle(dart_null, NULL, 1024, NULL);
On 2014/03/06 17:17:39, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2213: Dart_DeleteWeakPersistentHandle(weak2);
On 2014/03/06 17:17:39, siva wrote:
> Ditto comment regarding check.

Done.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_state.h
File runtime/vm/dart_api_state.h (right):

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:202: void SetExternalAllocationSize(intptr_t size,
Heap* heap) {
On 2014/03/06 21:59:33, Ivan Posva wrote:
> SetExternalSize() to match the field.

Done.

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:266: intptr_t external_size_;
On 2014/03/06 17:17:39, siva wrote:
> Not for this CL but just a thought:
> Now that we have an external field I am wondering if we should steal a bit
from
> here to indicate if the handle is a prolog weak persistent handle or a regular
> weak persistent handle (current we tag the returned pointer).

Sounds like a good idea. It would simplify the code (which became slightly more
complicated after merging with your tagging-change).

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

https://codereview.chromium.org/187113003/diff/90001/runtime/vm/heap.h#newcod...
runtime/vm/heap.h:108: void FreeExternal(intptr_t size, RawObject* ref);
On 2014/03/06 17:17:39, siva wrote:
> Normally we try to avoid passing in raw pointers as parameters to functions
> unless they are special functions that are invoked during GC or snapshot
> writing. Why not change this API to
> 
> void AllocateExternal(intptr_t size, Space space);
> void FreeExternal(intptr_t size, Space space);

Done. I moved the space detection code into FinalizeablePersistentHandle. Do you
think it would make sense to have a RawObject::GetSpace/GetGeneration?

[siva, 2014-03-07 19:53:03]

LGTM.

We will have to make a corresponding change to dartium to ensure the build bots
don't go red.

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

https://codereview.chromium.org/187113003/diff/170001/runtime/vm/dart_api_sta...
runtime/vm/dart_api_state.h:257: bool is_prologue_weak) {
This should now become a private method I suppose as we do not want anybody
calling it directly.

[koda, 2014-03-07 20:46:53]

Should I add the Dartium change to this CL?

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

https://codereview.chromium.org/187113003/diff/170001/runtime/vm/dart_api_sta...
runtime/vm/dart_api_state.h:257: bool is_prologue_weak) {
On 2014/03/07 19:53:03, siva wrote:
> This should now become a private method I suppose as we do not want anybody
> calling it directly.

It was still called by the isolate shutdown code, which finalizes all weak
persistent handles. I've changed it to UpdateUnreachable.

By the way, the API docs don't really specify what should happen on shutdown
(though I suppose the object does become unreachable, in a sense). I added a
comment to Dart_ShutdownIsolate.

[koda, 2014-03-07 23:53:40]

Committed patchset #12 manually as r33467 (presubmit successful).