Lazy peer API.

Lazy peer API.

This change provides getters and setters for a lazily allocated field
associated with every heap allocated object.  It is a generalization
of the peer field of external string instances, external byte array
instances, and the smrck field of closure instances.

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

[Anton Muhin, 2012-09-13 06:27:07]

Overall: given all the subtelities of iterating over the map and removing some
elements, won't it be simpler just to create a new map and swap with the old
one?

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:4206: if (obj.IsSmi()) {
that's up to you, but this dependence on internal representation of integers
looks a bit sketchy, not sure if it'll impose a real problem.

Similarly, later there might be arrays which store unwrapped doubles and there
might be complicated cases/performance implications.

As a variant: maybe prohibit peers for some core types like bool/double/int/num?

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h
File runtime/vm/gc_marker.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h#newcode28
runtime/vm/gc_marker.h:28: explicit GCMarker(Heap* heap, std::map<RawObject*,
void*>* peer) :
nit: explicit can go away now

http://codereview.chromium.org/10905251/diff/1/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/scavenger.cc#newcod...
runtime/vm/scavenger.cc:529: heap_->SetPeer(raw_obj, peer);
That looks sketchy, but I am not sure I remember STL semantics correctly.

My concern is you modify the map while iterating over it.  So are there any
chances that you'll iterate over just added objects too?  That looks possible to
me, esp. if to-spaces resides in higher addresses.

And if all above is possible, won't that make peers disappear?

http://codereview.chromium.org/10905251/diff/1/runtime/vm/scavenger.cc#newcod...
runtime/vm/scavenger.cc:620: void Scavenger::SetPeer(RawObject* raw_obj, void*
peer) {
you've written this code once, maybe abstract away into PeerMap?

[siva, 2012-09-14 00:03:45]

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2618: * its peer.
Do we really want to allow peer fields for every heap allocated object? e.g:
ints, doubles, bool etc.

In addition we already have a bunch of External* objects which seem to already
have corresponding native values, do these also get peer fields?

Will only some objects of a particular type have peer fields or do we except all
objects created of a particular type to have peer fields. I am trying to figure
out why somebody could not just extend NativeWrapper and store the peer value in
the native field.

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2623: * returns an error if the peer field has not
been allocated.
Why do you return an error if there is no peer field for an
object. If you set 'peer' to NULL and returned success that
would be an indication that the object does not have a peer.

Currently one has to make two calls:
  result = Dart_HasPeer(obj)
  if (result == success) {
    result = Dart_GetPeer(obj, peer);
    if (result == success) {
       do something;
    }
  }

Would be preferable if I could just make one call:

  result = Dart_GetPeer(obj, peer);
  if (result == success && *peer != NULL) {
    do something;
  }

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2636: * 'peer' is NULL the peer field is deallocated.
I find the notion of deleting a peer field by calling
Dart_SetPeer with a NULL peer value a bit overloaded.

Why not have a Dart_[Remove | Delete]Peer entry point.

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2648: DART_EXPORT bool Dart_HasPeer(Dart_Handle
object);
This call would not be needed if we go with the suggestion above.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test.cc
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6536: // the peer and check that the count of
peer objects is decremented by
checks

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6560: // Allocates an object in old space and
assigns it a peer.  Allow the
in new space

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6594: // Allocates two objects in old space and
assigns them peers.  Removes
in new space

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6679: 
I think we also need a test case where the object has not been promoted but
moved in new space. In fact we should do this test with at least 10 or so
objects with peers so that we can test the case that Anton has commented about.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6707: EXPECT(out ==
reinterpret_cast<void*>(&peer));
EXPECT_EQ(1, isolate->heap()->PeerCount());

To check new/old space counts is working.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6717: TEST_CASE(OneOldSpacePeer) {
The code below is identical to the one in OneNewSpacePeer
except for the use of Heap::kOld Vs Heap::kNew.

Why not abstract this into a static method with space as a parameter and use in
both places.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6747: TEST_CASE(CollectOneOldSpacePeer) {
Ditto comment about sharing with CollectOneNewSpacePeer

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6781: TEST_CASE(TwoOldSpacePeers) {
Ditto comment.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6825: TEST_CASE(CollectTwoOldSpacePeers) {
Ditto comment.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h
File runtime/vm/gc_marker.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h#newcode8
runtime/vm/gc_marker.h:8: #include <map>
I would prefer if we could have a local map and not have to use map from STL but
that is just my preference.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h#newcode51
runtime/vm/gc_marker.h:51: std::map<RawObject*, void*>* peer_;
why not call it peer_table_ or peer_map_

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

http://codereview.chromium.org/10905251/diff/1/runtime/vm/heap.h#newcode19
runtime/vm/heap.h:19: class Object;
Why is this forward declaration needed? I don't see any reference to Object in
thew new code.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/pages.h
File runtime/vm/pages.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/pages.h#newcode240
runtime/vm/pages.h:240: std::map<RawObject*, void*> peer_;
Call this peer_table_ or peer_map_;

http://codereview.chromium.org/10905251/diff/1/runtime/vm/scavenger.h
File runtime/vm/scavenger.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/scavenger.h#newcode144
runtime/vm/scavenger.h:144: std::map<RawObject*, void*> peer_;
peer_table_ or peer_map_ ?

[cshapiro, 2012-09-15 01:23:41]

PTAL

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2616: /**
I will add a comment here that peers are intended for low-frequency use ONLY.

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2618: * its peer.
I have limited this to types which are not subtypes of Null, num, and bool.

A subsequent revision of this facility will make use of unused words after an
object to store a pointer to a lazily allocated field structure which can
contain the peer value, hash code, and so forth.

Until this is finished some objects will have more than one "peer".  While
confusing, this should be a temporary condition.

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2623: * returns an error if the peer field has not
been allocated.
I am confused.  You can use second form.  The alternative would be completely
hide the laziness in the first place.  I have been thinking that might be a
better way to go.

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2636: * 'peer' is NULL the peer field is deallocated.
The peer field being deleted is an implementation detail.  I think it really
doesn't make a difference as the getter always returns NULL.

http://codereview.chromium.org/10905251/diff/1/runtime/include/dart_api.h#new...
runtime/include/dart_api.h:2648: DART_EXPORT bool Dart_HasPeer(Dart_Handle
object);
It's gone.  Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl.cc#ne...
runtime/vm/dart_api_impl.cc:4206: if (obj.IsSmi()) {
I think your suggestion to prohibit the bool type and the num subtypes is very
attractive.  I have also added a test case for these things.  Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test.cc
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6536: // the peer and check that the count of
peer objects is decremented by
On 2012/09/14 00:03:46, siva wrote:
> checks

Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6560: // Allocates an object in old space and
assigns it a peer.  Allow the
On 2012/09/14 00:03:46, siva wrote:
> in new space

Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6594: // Allocates two objects in old space and
assigns them peers.  Removes
On 2012/09/14 00:03:46, siva wrote:
> in new space

Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6679: 
Good idea.  Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6707: EXPECT(out ==
reinterpret_cast<void*>(&peer));
Good idea.  Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6717: TEST_CASE(OneOldSpacePeer) {
We could.  But, I expect this to acquire space-specific behavior once we store
peer objects adjacent to heap objects instead of in a table off to the side.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6781: TEST_CASE(TwoOldSpacePeers) {
On 2012/09/14 00:03:46, siva wrote:
> Ditto comment.

Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/dart_api_impl_test....
runtime/vm/dart_api_impl_test.cc:6825: TEST_CASE(CollectTwoOldSpacePeers) {
On 2012/09/14 00:03:46, siva wrote:
> Ditto comment.

Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h
File runtime/vm/gc_marker.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h#newcode28
runtime/vm/gc_marker.h:28: explicit GCMarker(Heap* heap, std::map<RawObject*,
void*>* peer) :
Thanks.  Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/gc_marker.h#newcode51
runtime/vm/gc_marker.h:51: std::map<RawObject*, void*>* peer_;
Renamed.  Done.

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

http://codereview.chromium.org/10905251/diff/1/runtime/vm/heap.h#newcode19
runtime/vm/heap.h:19: class Object;
This forward reference for Object is noise.  Removed.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/pages.h
File runtime/vm/pages.h (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/pages.h#newcode240
runtime/vm/pages.h:240: std::map<RawObject*, void*> peer_;
Done.

http://codereview.chromium.org/10905251/diff/1/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

http://codereview.chromium.org/10905251/diff/1/runtime/vm/scavenger.cc#newcod...
runtime/vm/scavenger.cc:529: heap_->SetPeer(raw_obj, peer);
Modifying the map while traversing it should be okay as far as the STL is
concerned.  However, as you have observed, this is not correct as far as the
algorithm is concerned.

Using a separate table has some additional advantages.  I will make that change.

[Anton Muhin, 2012-09-17 14:23:36]

lgtm

[Anton Muhin, 2012-10-10 14:54:07]

Apparently I've spotted a problem with this CL

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4456: const Object& obj = Object::Handle(isolate,
Api::UnwrapHandle(object));
isn't DARTSCOPE(isolate) missing here and in the second method?

[siva, 2012-10-10 15:01:28]

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4456: const Object& obj = Object::Handle(isolate,
Api::UnwrapHandle(object));
Yes.

And a test case that will exercise a path where we do not implicitly set up
Zone/Handlescope.

Thanks for catching that.

On 2012/10/10 14:54:08, Anton Muhin wrote:
> isn't DARTSCOPE(isolate) missing here and in the second method?

[siva, 2012-10-10 15:01:28]

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4456: const Object& obj = Object::Handle(isolate,
Api::UnwrapHandle(object));
Yes.

And a test case that will exercise a path where we do not implicitly set up
Zone/Handlescope.

Thanks for catching that.

On 2012/10/10 14:54:08, Anton Muhin wrote:
> isn't DARTSCOPE(isolate) missing here and in the second method?

[cshapiro, 2012-10-10 16:39:07]

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10905251/diff/11001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4456: const Object& obj = Object::Handle(isolate,
Api::UnwrapHandle(object));
Thanks.  I will add that today to both GetPeer and SetPeer.