Object ID Ring with tests

Object ID Ring with tests

R=iposva@google.com

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

[Cutch, 2013-07-09 20:51:45]

Patch Set 2:

* Merged to bleeding_edge.
* Switch from intptr_t to int32_t.
* IDs are always within smi range on 32-bit machines.

PTAL.

[Ivan Posva, 2013-07-10 01:10:13]

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

https://codereview.chromium.org/18259014/diff/1/runtime/vm/object_id_ring.cc#...
runtime/vm/object_id_ring.cc:39: return Object::null();
We might want to return an object that is known to have been collected on the
other side of the protocol. For example an object marked as invalid.

https://codereview.chromium.org/18259014/diff/1/runtime/vm/object_id_ring.cc#...
runtime/vm/object_id_ring.cc:77: ASSERT(raw_obj->IsHeapObject());
How do you intend to represent Smis on the wire?

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/dart.cc#newcode114
runtime/vm/dart.cc:114: ObjectIdRing::Init(vm_isolate_);
Does the VM isolate really need an object id ring?

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

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/heap.h#newcode234
runtime/vm/heap.h:234: void set_object_id_ring_table(RawObject**
object_id_ring_table,
How about set_object_id_table and object_id_table()?

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/heap.h#newcode240
runtime/vm/heap.h:240: RawObject** get_object_id_ring_table() {
Why does this return the naked internals of the ObjectIdRing object and not the
ObjectIdRing? Do you really need it here, or why does the code that needs access
to the ObjectIdRing not get it from the isolate itself?

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:1074: void Isolate::set_object_id_ring(ObjectIdRing* ring)
{
If the code is this involved and not in the header, then it should be CamelCase
as it is not a trivial field accessor any longer. In this case I do think we can
remove the setting of the heap fields as noted elsewhere which means it becomes
a trivial setter again.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.h#newco...
runtime/vm/isolate.h:627: void set_object_id_ring(ObjectIdRing* ring);
Please put this up above with the other trivial accessors. Also it is
object_id_ring() and set_object_id_ring(), not the missing get_.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring.cc
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring....
runtime/vm/object_id_ring.cc:12: #define ID_MAX 0x3FFFFFFF
Probably better as a const int32_t kIdMax in class ObjectIdRing.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring....
runtime/vm/object_id_ring.cc:16: if (isolate != NULL) {
In which case would the isolate be NULL?

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring....
runtime/vm/object_id_ring.cc:56: const size_t table_size =
sizeof(sizer)*capacity_;
kWordSize * capacity_ or even better use calloc below.

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

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/scavenger.cc#ne...
runtime/vm/scavenger.cc:606: RawObject** table =
heap_->get_object_id_ring_table();
It would be better if we used a pointer visitor here instead of accessing the
internals of the ObjectIdRing directly.

[Cutch, 2013-07-10 17:23:10]

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

https://codereview.chromium.org/18259014/diff/1/runtime/vm/object_id_ring.cc#...
runtime/vm/object_id_ring.cc:39: return Object::null();
On 2013/07/10 01:10:13, Ivan Posva wrote:
> We might want to return an object that is known to have been collected on the
> other side of the protocol. For example an object marked as invalid.

Do we want to distinguish between the following two cases?

#1 Object was GC'd
#2 ID is no longer (or never was) valid

https://codereview.chromium.org/18259014/diff/1/runtime/vm/object_id_ring.cc#...
runtime/vm/object_id_ring.cc:77: ASSERT(raw_obj->IsHeapObject());
On 2013/07/10 01:10:13, Ivan Posva wrote:
> How do you intend to represent Smis on the wire?

When sending objects as JSON there is always a "type" field. For numbers a
message would look like this:

{
"type": "Integer",
"value": 99
}

{
"type": "Double",
"value": 3.14159
}

When referencing an object the type is prefixed with an "@", for example:

{
"type": "@String",
"id": 3
}

If we then requested the String with id three the response would look like:

{
"type": "String",
"value": "Hello World!"
}

Code that serializes objects into JSON should not request an ID for simple
scalar types (e.g. bool, int, double, Float32x4, and Uint32x4) and instead
always send the value.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/dart.cc#newcode114
runtime/vm/dart.cc:114: ObjectIdRing::Init(vm_isolate_);
On 2013/07/10 01:10:13, Ivan Posva wrote:
> Does the VM isolate really need an object id ring?

Done.

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

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/heap.h#newcode234
runtime/vm/heap.h:234: void set_object_id_ring_table(RawObject**
object_id_ring_table,
On 2013/07/10 01:10:13, Ivan Posva wrote:
> How about set_object_id_table and object_id_table()?

Done.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/heap.h#newcode240
runtime/vm/heap.h:240: RawObject** get_object_id_ring_table() {
On 2013/07/10 01:10:13, Ivan Posva wrote:
> Why does this return the naked internals of the ObjectIdRing object and not
the
> ObjectIdRing? Do you really need it here, or why does the code that needs
access
> to the ObjectIdRing not get it from the isolate itself?

I've killed this and now the marker and scavenger get the ring from the isolate
directly.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:1074: void Isolate::set_object_id_ring(ObjectIdRing* ring)
{
On 2013/07/10 01:10:13, Ivan Posva wrote:
> If the code is this involved and not in the header, then it should be
CamelCase
> as it is not a trivial field accessor any longer. In this case I do think we
can
> remove the setting of the heap fields as noted elsewhere which means it
becomes
> a trivial setter again.

Done.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/isolate.h#newco...
runtime/vm/isolate.h:627: void set_object_id_ring(ObjectIdRing* ring);
On 2013/07/10 01:10:13, Ivan Posva wrote:
> Please put this up above with the other trivial accessors. Also it is
> object_id_ring() and set_object_id_ring(), not the missing get_.

Done.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring.cc
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring....
runtime/vm/object_id_ring.cc:12: #define ID_MAX 0x3FFFFFFF
On 2013/07/10 01:10:13, Ivan Posva wrote:
> Probably better as a const int32_t kIdMax in class ObjectIdRing.

Done.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring....
runtime/vm/object_id_ring.cc:16: if (isolate != NULL) {
On 2013/07/10 01:10:13, Ivan Posva wrote:
> In which case would the isolate be NULL?

In a unit test.

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/object_id_ring....
runtime/vm/object_id_ring.cc:56: const size_t table_size =
sizeof(sizer)*capacity_;
On 2013/07/10 01:10:13, Ivan Posva wrote:
> kWordSize * capacity_ or even better use calloc below.

Done.

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

https://codereview.chromium.org/18259014/diff/3001/runtime/vm/scavenger.cc#ne...
runtime/vm/scavenger.cc:606: RawObject** table =
heap_->get_object_id_ring_table();
On 2013/07/10 01:10:13, Ivan Posva wrote:
> It would be better if we used a pointer visitor here instead of accessing the
> internals of the ObjectIdRing directly.

All accesses to the ring table via the scavenger and the marker are now done
using an ObjectPointerVisitor.

[Ivan Posva, 2013-07-11 17:01:42]

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

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/gc_marker.cc#n...
runtime/vm/gc_marker.cc:411: if (!raw_obj->IsMarked()) {
You need to be checking the mark bits on old objects only! You do not know what
these bits indicate for new gen objects. In the current system this will lead to
an old gc clearing ALL new objects which is not what you want, they should not
be touched at all.

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/gc_marker.cc#n...
runtime/vm/gc_marker.cc:419: Clear(current);
You can move the logic from Clear in here. The name Clear is a bit confusing as
it implies that you are clearing all entries. I don't think the extra method is
adding much to clarity.

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

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/heap.h#newcode296
runtime/vm/heap.h:296: // Ring of objects who have a vm service id assigned to
them.
Is this still needed?

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/object_id_ring.cc
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/object_id_ring...
runtime/vm/object_id_ring.cc:13: if (isolate != NULL) {
I'd prefer it if the test case for wrapping was using a regular isolate adding
real values into the object id ring instead of Object::null which is also the
default empty value. You could set the current index as part of the testcase.

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/object_id_ring...
runtime/vm/object_id_ring.cc:47: if (table_ != NULL) {
ASSERT(table_ != NULL);

I don't see a way table_ should ever be NULL with a correctly functioning setup.

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

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/scavenger.cc#n...
runtime/vm/scavenger.cc:693: ProcessObjectIdTable(isolate);
It is unclear to me why you need this here and all the machinery associated with
it. You have already processed the object id ring above in IterateRoots.

[Cutch, 2013-07-11 21:31:47]

PTAL

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

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/gc_marker.cc#n...
runtime/vm/gc_marker.cc:411: if (!raw_obj->IsMarked()) {
On 2013/07/11 17:01:42, Ivan Posva wrote:
> You need to be checking the mark bits on old objects only! You do not know
what
> these bits indicate for new gen objects. In the current system this will lead
to
> an old gc clearing ALL new objects which is not what you want, they should not
> be touched at all.

Done.

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/gc_marker.cc#n...
runtime/vm/gc_marker.cc:419: Clear(current);
On 2013/07/11 17:01:42, Ivan Posva wrote:
> You can move the logic from Clear in here. The name Clear is a bit confusing
as
> it implies that you are clearing all entries. I don't think the extra method
is
> adding much to clarity.

Done.

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

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/heap.h#newcode296
runtime/vm/heap.h:296: // Ring of objects who have a vm service id assigned to
them.
On 2013/07/11 17:01:42, Ivan Posva wrote:
> Is this still needed?

Done.

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/object_id_ring.cc
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/object_id_ring...
runtime/vm/object_id_ring.cc:13: if (isolate != NULL) {
On 2013/07/11 17:01:42, Ivan Posva wrote:
> I'd prefer it if the test case for wrapping was using a regular isolate adding
> real values into the object id ring instead of Object::null which is also the
> default empty value. You could set the current index as part of the testcase.

Done.

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/object_id_ring...
runtime/vm/object_id_ring.cc:47: if (table_ != NULL) {
On 2013/07/11 17:01:42, Ivan Posva wrote:
> ASSERT(table_ != NULL);
> 
> I don't see a way table_ should ever be NULL with a correctly functioning
setup.

Done.

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

https://codereview.chromium.org/18259014/diff/13001/runtime/vm/scavenger.cc#n...
runtime/vm/scavenger.cc:693: ProcessObjectIdTable(isolate);
On 2013/07/11 17:01:42, Ivan Posva wrote:
> It is unclear to me why you need this here and all the machinery associated
with
> it. You have already processed the object id ring above in IterateRoots.

You are correct.

[Ivan Posva, 2013-07-16 15:32:55]

LGTM -ip

[Cutch, 2013-07-16 18:32:17]

Committed patchset #5 manually as r25063 (presubmit successful).