Support receiving GrowableObjectArray on native ports

Support receiving GrowableObjectArray on native ports

Array literals are created as GrowableObjectArrays internally.

A GrowableObjectArray holds a length and a backing store. The backing
store is an array. When receiving a GrowableObjectArray an object is
created for holding the GrowableObjectArray. The the backing store has
been read the object holding the GrowableObjectArray is transformed
into an array with its content from the backing store.

R=asiva@google.com, ager@google.com

BUG=dart:4993
TEST=runtime/vm/snapshot_test.cc/DartGeneratedArrayLiteralMessagesWithBackref,
     runtime/vm/snapshot_test.cc/DartGeneratedArrayLiteralMessages

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

[Mads Ager (google), 2012-09-10 13:28:18]

LGTM, but we should wait for Siva's comments.

https://chromiumcodereview.appspot.com/10916207/diff/1/runtime/vm/dart_api_me...
File runtime/vm/dart_api_message.cc (right):

https://chromiumcodereview.appspot.com/10916207/diff/1/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.cc:167: // Allocate a Dart_CObject structure for
holding the GrowableObjectArray
Move this comment to before the method as it is a method comment?

https://chromiumcodereview.appspot.com/10916207/diff/1/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.cc:239: // Make the GrowableObjectArray read into an
array.
I find this comment confusing. Maybe add comments in the style of the method
comment for the partial allocation? Something along the lines of:

if (value == NULL) {
  // Allocate a partially initialized array
  // CObject structure.
  ...
}
// Read the content of the GrowableObjectArray.
...
// Finish the initialization of the array CObject structure.

[Søren Gjesse, 2012-09-10 14:02:02]

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

http://codereview.chromium.org/10916207/diff/1/runtime/vm/dart_api_message.cc...
runtime/vm/dart_api_message.cc:167: // Allocate a Dart_CObject structure for
holding the GrowableObjectArray
On 2012/09/10 13:28:18, Mads Ager wrote:
> Move this comment to before the method as it is a method comment?

Ended up removing this method.

http://codereview.chromium.org/10916207/diff/1/runtime/vm/dart_api_message.cc...
runtime/vm/dart_api_message.cc:239: // Make the GrowableObjectArray read into an
array.
On 2012/09/10 13:28:18, Mads Ager wrote:
> I find this comment confusing. Maybe add comments in the style of the method
> comment for the partial allocation? Something along the lines of:
> 
> if (value == NULL) {
>   // Allocate a partially initialized array
>   // CObject structure.
>   ...
> }
> // Read the content of the GrowableObjectArray.
> ...
> // Finish the initialization of the array CObject structure.

Simplified to just allocate an empty array for the GrowableObjectArray and
update length and values fields after the backing store is read.

[Mads Ager (google), 2012-09-10 14:05:04]

Even better! :)

http://codereview.chromium.org/10916207/diff/4001/runtime/vm/dart_api_message.cc
File runtime/vm/dart_api_message.cc (right):

http://codereview.chromium.org/10916207/diff/4001/runtime/vm/dart_api_message...
runtime/vm/dart_api_message.cc:216: // Allocate an empty array for the
GrowableObjectArray this will be updated
this will -> which will

[siva, 2012-09-11 01:46:23]

LGTM with question regarding how free of this
growable array object should be done if the allocator
used is a malloc based allocator?

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/dart_api...
File runtime/vm/dart_api_message.cc (right):

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/dart_api...
runtime/vm/dart_api_message.cc:225: value->value.as_array.values =
content->value.as_array.values;
How will this work if the allocator specified in the constructor for
ApiMessageReader is a malloc allocator?

A free of the returned object will not deallocate the backing store right.

Agreed that we currently only use a zone allocator in the
native message handler and all the test cases and this is
not an issue with a zone allocator.

The ApiMessageHandler class itself doesn't specify that only a zone allocator
should be used.

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/snapshot...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/snapshot...
runtime/vm/snapshot_test.cc:1684: EXPECT_EQ(Dart_CObject::kInt64,
element->type);
In the string case you had a verification that element was "Hello, world!" but
here there is no such check against an
expected value.

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/snapshot...
runtime/vm/snapshot_test.cc:1697: EXPECT_EQ(Dart_CObject::kBigint,
element->type);
Ditto comment regarding check against expected value.

[Søren Gjesse, 2012-09-11 14:36:03]

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/dart_api...
File runtime/vm/dart_api_message.cc (right):

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/dart_api...
runtime/vm/dart_api_message.cc:216: // Allocate an empty array for the
GrowableObjectArray this will be updated
On 2012/09/10 14:05:05, Mads Ager wrote:
> this will -> which will

Done.

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/dart_api...
runtime/vm/dart_api_message.cc:225: value->value.as_array.values =
content->value.as_array.values;
On 2012/09/11 01:46:23, siva wrote:
> How will this work if the allocator specified in the constructor for
> ApiMessageReader is a malloc allocator?
> 
> A free of the returned object will not deallocate the backing store right.
> 
> Agreed that we currently only use a zone allocator in the
> native message handler and all the test cases and this is
> not an issue with a zone allocator.
> 
> The ApiMessageHandler class itself doesn't specify that only a zone allocator
> should be used.
> 

We don't really have a way of running through the decoded structure to free the
individual pieces. This was already the case before as the decoded structure can
have cycles and there is no specification of how the various pieces for the
decoded structure are allocated.

I added a comment to the ApiMessageReader saying that the allocator needs to
keep track of data allocated to be able to free it again.

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/snapshot...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/snapshot...
runtime/vm/snapshot_test.cc:1684: EXPECT_EQ(Dart_CObject::kInt64,
element->type);
On 2012/09/11 01:46:23, siva wrote:
> In the string case you had a verification that element was "Hello, world!" but
> here there is no such check against an
> expected value.

Done.

https://chromiumcodereview.appspot.com/10916207/diff/4001/runtime/vm/snapshot...
runtime/vm/snapshot_test.cc:1697: EXPECT_EQ(Dart_CObject::kBigint,
element->type);
On 2012/09/11 01:46:23, siva wrote:
> Ditto comment regarding check against expected value.

Done.