Add a first class GrowableObjectArray type in the VM and use it internally in the VM at all spots w…

Add a first class GrowableObjectArray type in the VM and use it internally in the VM at all spots were we use GrowableArray first and then copy the contents into an Array object.

Next step is to use this type in growable_array.dart
Committed: https://code.google.com/p/dart/source/detail?r=5109

[hausner, 2012-03-06 00:30:46]

The main issue i have is with handle allocation for each array element access.
Otherwise lgtm.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc
File vm/object.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:448: // class is setup as one of it's field is an array object.
it's -> its

Also missing ) in comment.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:7982: if (capacity_size != used_size) {
Will this filler array/object be collected by the GC? Neat.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:8065: if (!AbstractTypeArguments::AreEqual(
Would it make sense to compare the length first? I wouldn't be surprised if the
type arguments would often by "Dynamic" on both arrays, but the size may be
different.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3357: const Array& contents = Array::Handle(data());
Is there no way around accessing a growable array without allocating a handle on
each access?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc
File vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc#newcod...
vm/parser.cc:526: obj ^= list.At(i);
NB: each one of these accesses is currently allocating a handle!

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc#newcod...
vm/parser.cc:550: GrowableObjectArray& fields;
If the handle allocation in GrowableObjectArray::At can't be solved, it may make
more sense to keep these lists the way they are.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc#newcod...
vm/parser.cc:3466: GrowableObjectArray::Handle(isolate,
object_store->pending_classes());
It's nice that you avoid copying the parsed classes from a local list in the
parser to a list in the finalizer. The only issue I see is that we might end up
with partially parsed classes in the finalizer if there is a compilation error.
Currently, that means the whole isolate is killed, but maybe in the future we
want to be able to survive some compilation errors.

[cshapiro, 2012-03-06 00:56:31]

Where is TwoByteGrowableObjectArray and FourByteGrowableObjectArray?

:-)

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer.cc
File vm/class_finalizer.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer....
vm/class_finalizer.cc:49: class_array = object_store->pending_classes();
Any reason this isn't a ^= assignment?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer....
vm/class_finalizer.cc:83: class_array = GrowableObjectArray::New();
Same here regarding ^=

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer....
vm/class_finalizer.cc:127: Class& interface_class = Class::Handle();
Any particular reason to pull these out of the loop?  Efficiency?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc
File vm/object.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:8028: intptr_t new_capacity = Capacity() * 2;
Overflow check?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3302: // Make an Array object from a growable Array by truncating it
to the
This is a bit confusing to me.  A growable array contains an array, right?  If
so, I would imagine this returning the underlying array object, right?  What
happens to the growable array?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3343: class GrowableObjectArray : public Instance {
Class documentation?  (I am not so good about this myself, feel free to bug me
too.)

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object.h
File vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object.h#new...
vm/raw_object.h:1011: intptr_t capacity_;
I am not sure why we need a separate capacity_ member when we have the array
length in data_.  Is there ever a situation where these values are not equal?

Presumably, these size fields are Smi instances.  I have been lead to believe
that we should scan those like other reference values.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object_snaps...
File vm/raw_object_snapshot.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object_snaps...
vm/raw_object_snapshot.cc:1719: // Read the length so that we can determine
instance size to allocate.
Maybe use the common array reader.  See my comment below.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object_snaps...
vm/raw_object_snapshot.cc:1747: writer->WriteIntptrValue(ptr()->capacity_);
Hmm... if you get rid of the capacity member you can use ArrayWriteTo here.

The length value just has to be the length of the growable array and you can
write out just that many elements.

The snapshot reader will allocate a fixed array with enough headroom to grow on
the other side.  There is no need to write out the fixed array separately.

[siva, 2012-03-06 23:32:33]

Addressed review comments, PTAL.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer.cc
File vm/class_finalizer.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer....
vm/class_finalizer.cc:49: class_array = object_store->pending_classes();
pending_classes is declared to be of type RawGrowableObjectArray and hence
should return that type and does not need a cast.

On 2012/03/06 00:56:31, cshapiro wrote:
> Any reason this isn't a ^= assignment?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer....
vm/class_finalizer.cc:83: class_array = GrowableObjectArray::New();
Same here GrowableObjectArray::New() returns an object of type
RawGrowableObjectArray and does not need a cast.

On 2012/03/06 00:56:31, cshapiro wrote:
> Same here regarding ^=

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/class_finalizer....
vm/class_finalizer.cc:127: Class& interface_class = Class::Handle();
Yes efficiency. Don't want to keep creating handles in the loop and instead just
reuse the handle created here.

On 2012/03/06 00:56:31, cshapiro wrote:
> Any particular reason to pull these out of the loop?  Efficiency?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc
File vm/object.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:448: // class is setup as one of it's field is an array object.
On 2012/03/06 00:30:47, hausner wrote:
> it's -> its
> 
> Also missing ) in comment.

Done.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:7982: if (capacity_size != used_size) {
Yes, this would be an unreferenced object and hence would get collected.

On 2012/03/06 00:30:47, hausner wrote:
> Will this filler array/object be collected by the GC? Neat.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:8028: intptr_t new_capacity = Capacity() * 2;
On 2012/03/06 00:56:31, cshapiro wrote:
> Overflow check?

I added a check to throw an exception if new_capacity is smaller than the old
capacity.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.cc#newcod...
vm/object.cc:8065: if (!AbstractTypeArguments::AreEqual(
On 2012/03/06 00:30:47, hausner wrote:
> Would it make sense to compare the length first? I wouldn't be surprised if
the
> type arguments would often by "Dynamic" on both arrays, but the size may be
> different.

Done.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3302: // Make an Array object from a growable Array by truncating it
to the
On 2012/03/06 00:56:31, cshapiro wrote:
> This is a bit confusing to me.  A growable array contains an array, right?  If
> so, I would imagine this returning the underlying array object, right?  What
> happens to the growable array?

Yes it returns the underlying array but truncates it to the appropriate size
(used size).
The data part of the growable array is set to an empty array.

I have reworded the comment, see if it makes more sense now.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3343: class GrowableObjectArray : public Instance {
I was trying to stick to the comment style used in this file for classes.

On 2012/03/06 00:56:31, cshapiro wrote:
> Class documentation?  (I am not so good about this myself, feel free to bug me
> too.)

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3357: const Array& contents = Array::Handle(data());
I have modified this code to avoid creation of the handle.

On 2012/03/06 00:30:47, hausner wrote:
> Is there no way around accessing a growable array without allocating a handle
on
> each access?

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc
File vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc#newcod...
vm/parser.cc:526: obj ^= list.At(i);
On 2012/03/06 00:30:47, hausner wrote:
> NB: each one of these accesses is currently allocating a handle!

I have removed the handle creation in At(i);

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc#newcod...
vm/parser.cc:550: GrowableObjectArray& fields;
I have removed the handle creation in At(...);

On 2012/03/06 00:30:47, hausner wrote:
> If the handle allocation in GrowableObjectArray::At can't be solved, it may
make
> more sense to keep these lists the way they are.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/parser.cc#newcod...
vm/parser.cc:3466: GrowableObjectArray::Handle(isolate,
object_store->pending_classes());
Good point.

If we switch to a mode where we want to survive some compilation errors we could
add some state in the compilation unit or class indicating that it has
compilation errors and that state can be checked in the class finalizer to skip
over all the classes that have errors. 

On 2012/03/06 00:30:47, hausner wrote:
> It's nice that you avoid copying the parsed classes from a local list in the
> parser to a list in the finalizer. The only issue I see is that we might end
up
> with partially parsed classes in the finalizer if there is a compilation
error.
> Currently, that means the whole isolate is killed, but maybe in the future we
> want to be able to survive some compilation errors.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object.h
File vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object.h#new...
vm/raw_object.h:1011: intptr_t capacity_;
Removed the capacity_ field and made length_ a Smi.

On 2012/03/06 00:56:31, cshapiro wrote:
> I am not sure why we need a separate capacity_ member when we have the array
> length in data_.  Is there ever a situation where these values are not equal?
> 
> Presumably, these size fields are Smi instances.  I have been lead to believe
> that we should scan those like other reference values.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object_snaps...
File vm/raw_object_snapshot.cc (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object_snaps...
vm/raw_object_snapshot.cc:1719: // Read the length so that we can determine
instance size to allocate.
See my comment below.

On 2012/03/06 00:56:31, cshapiro wrote:
> Maybe use the common array reader.  See my comment below.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/raw_object_snaps...
vm/raw_object_snapshot.cc:1747: writer->WriteIntptrValue(ptr()->capacity_);
I got rid of the capacity_ field.

I prefer to write out the fixed array as an object here instead of using
ArrayWriteTo because I don't then have to make RawGrowableObjectArray a friend
class of RawArray and add that additional dependency that any changes to
RawArray should also be reflected in the code here.

The only downside to this is some Null objects get written out but these are
just an index and very efficient that I feel it wouldn't become a bottleneck.

On 2012/03/06 00:56:31, cshapiro wrote:
> Hmm... if you get rid of the capacity member you can use ArrayWriteTo here.
> 
> The length value just has to be the length of the growable array and you can
> write out just that many elements.
> 
> The snapshot reader will allocate a fixed array with enough headroom to grow
on
> the other side.  There is no need to write out the fixed array separately.

[hausner, 2012-03-06 23:53:58]

LVGTM.

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3357: const Array& contents = Array::Handle(data());
Nice, thank you.

On 2012/03/06 23:32:33, asiva wrote:
> I have modified this code to avoid creation of the handle.
> 
> On 2012/03/06 00:30:47, hausner wrote:
> > Is there no way around accessing a growable array without allocating a
handle
> on
> > each access?
>

[cshapiro, 2012-03-07 00:12:53]

lgtm

consider revising the comment in object.h

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3302: // Make an Array object from a growable Array by truncating it
to the
This is much better, however it does not mention that the backing array is
returned.  I would modify the second sentence to say, 

  This is done by first truncating the Growable Object Array's backing array to
the currently used size and then returning the truncated backing array.

[siva, 2012-03-08 00:51:56]

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9594028/diff/13003/vm/object.h#newcode...
vm/object.h:3302: // Make an Array object from a growable Array by truncating it
to the
On 2012/03/07 00:12:53, cshapiro wrote:
> This is much better, however it does not mention that the backing array is
> returned.  I would modify the second sentence to say, 
> 
>   This is done by first truncating the Growable Object Array's backing array
to
> the currently used size and then returning the truncated backing array.

Done.