Allocate a Dart_CMessage structure when decoding a message into C structures

Allocate a Dart_CMessage structure when decoding a message into C structures

Changed tests to use zone allocation for C structures.

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

BUG=
TEST=

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

[Søren Gjesse, 2012-01-31 14:46:48]

This is based on https://chromiumcodereview.appspot.com/9303031/

[turnidge, 2012-01-31 22:20:42]

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

http://codereview.chromium.org/9159066/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:1437: Allocator allocator);
Do we need this to be a public api?  If the Dart_NativePortHandler function
takes a Dart_CMessage as an argument, then the decoding, allocation, and
deallocation would happen internally in the vm.

[siva, 2012-02-01 01:48:36]

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1419: Dart_CObject** allocated;
Do we need the allocated and allocated_length fields in this strucuture.
If we use the native handler callback API provided by Todd and allow the message
handler loop to free up the allocated CMessage objects (using some kind of zone
allocator) this part would be a lot simpler.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1421: intptr_t original_message_length;
Do we need the original message and length etc. over here?

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1437: Allocator allocator);
On 2012/01/31 22:20:42, turnidge wrote:
> Do we need this to be a public api?  If the Dart_NativePortHandler function
> takes a Dart_CMessage as an argument, then the decoding, allocation, and
> deallocation would happen internally in the vm.


I agree with Todd, the native message handler could be made to take the decoded
CMessage object and the message loop would do the deserialization of the
message.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:477: if (message == NULL) return NULL;
The null check for message should happen before message is accessed in the line
above?

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:482: }
Not sure if we need to do this copying over here. Please see comment in
dart_api.h

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:70: // Read object back from the snapshot into a C
structure.
If you start a Zone here and use a zone allocator method then you don't have to
implement the FreeDeserializedDartMessage function.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:105: FreeDeserializedDartMessage(cmessage);
Ditto comment here and below about not needing FreeDeserializedDartMessage

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:195: USE(snapshot);
Why not just Snapshot::SetupFromBuffer(buffer);

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:219: USE(snapshot);
Ditto.

[Søren Gjesse, 2012-02-01 12:12:23]

PTAL

I have now removed the public API for decoding messages, so this CL is now
mainly adding ReadMessage to the CMessageReader.

I changed the tests to use zone allocator for the decoded message structures.
The generated messages are still allocated by a malloc based allocator as before
(wnere each test leaks the message).

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1419: Dart_CObject** allocated;
On 2012/02/01 01:48:36, asiva wrote:
> Do we need the allocated and allocated_length fields in this strucuture.
> If we use the native handler callback API provided by Todd and allow the
message
> handler loop to free up the allocated CMessage objects (using some kind of
zone
> allocator) this part would be a lot simpler.

We don't need it if we decide to handle the decoding and allocation/freeing
inside the VM.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1421: intptr_t original_message_length;
On 2012/02/01 01:48:36, asiva wrote:
> Do we need the original message and length etc. over here?

The reason they are here is that if we allow for the Dart_CObject structures to
point back into the original message (e.g. for large byte arrays) and the
embedder is responsible for freeing this structure then there need to be a
pointer to the original message which needs to be freed as well.

Maybe that makes it all too complicated for now.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1437: Allocator allocator);
On 2012/01/31 22:20:42, turnidge wrote:
> Do we need this to be a public api?  If the Dart_NativePortHandler function
> takes a Dart_CMessage as an argument, then the decoding, allocation, and
> deallocation would happen internally in the vm.

Maybe we don't. We could start out with delivering decoded messages with their
lifetime controlled by the VM. However that require that the receiver of a
decoded message does not keep around pointers but copies any data needed across
calls.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1437: Allocator allocator);
On 2012/02/01 01:48:36, asiva wrote:
> On 2012/01/31 22:20:42, turnidge wrote:
> > Do we need this to be a public api?  If the Dart_NativePortHandler function
> > takes a Dart_CMessage as an argument, then the decoding, allocation, and
> > deallocation would happen internally in the vm.
> 
> 
> I agree with Todd, the native message handler could be made to take the
decoded
> CMessage object and the message loop would do the deserialization of the
> message.

I am fine with this - we can always add embedder controlled decoding later if
required.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:477: if (message == NULL) return NULL;
On 2012/02/01 01:48:36, asiva wrote:
> The null check for message should happen before message is accessed in the
line
> above?

Done.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:482: }
On 2012/02/01 01:48:36, asiva wrote:
> Not sure if we need to do this copying over here. Please see comment in
> dart_api.h

Only needed if pointing back into the message and embedder is responsible for
freeing. Removed.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:70: // Read object back from the snapshot into a C
structure.
On 2012/02/01 01:48:36, asiva wrote:
> If you start a Zone here and use a zone allocator method then you don't have
to
> implement the FreeDeserializedDartMessage function.

Agree. But I wanted to cover the case where the embedder supplies an allocator
which is not zone but malloc based, and make sure that we provided sufficient
information for the embedder to correctly deallocate all allocated data.

Removed for now and use a zone allocator for allocating the structures for the
message decoding.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:105: FreeDeserializedDartMessage(cmessage);
On 2012/02/01 01:48:36, asiva wrote:
> Ditto comment here and below about not needing FreeDeserializedDartMessage

See above.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:195: USE(snapshot);
On 2012/02/01 01:48:36, asiva wrote:
> Why not just Snapshot::SetupFromBuffer(buffer);

Done.

https://chromiumcodereview.appspot.com/9159066/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:219: USE(snapshot);
On 2012/02/01 01:48:36, asiva wrote:
> Ditto.

Done.

[siva, 2012-02-02 03:46:45]

lgtm

https://chromiumcodereview.appspot.com/9159066/diff/9001/runtime/vm/snapshot_...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/9159066/diff/9001/runtime/vm/snapshot_...
runtime/vm/snapshot_test.cc:85: EXPECT_EQ(Dart_CObject::kNull, cobject->type);
I think there was a leak originally in this test case, a free of 'buffer' (The
buffer into which the snapshot was written into) was not being done.
Sorry about that.

free(buffer);

would be needed in all these unit tests that invoke the SnapshotWriter.

Or we could use the new zone_allocator that you have added as the allocator for
the SnapshotWriter too and move your Zone zone(...) declaration above the writer
object.

Your choice.

[Søren Gjesse, 2012-02-02 10:49:33]

http://codereview.chromium.org/9159066/diff/9001/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

http://codereview.chromium.org/9159066/diff/9001/runtime/vm/snapshot_test.cc#...
runtime/vm/snapshot_test.cc:85: EXPECT_EQ(Dart_CObject::kNull, cobject->type);
On 2012/02/02 03:46:45, asiva wrote:
> I think there was a leak originally in this test case, a free of 'buffer' (The
> buffer into which the snapshot was written into) was not being done.
> Sorry about that.
> 
> free(buffer);
> 
> would be needed in all these unit tests that invoke the SnapshotWriter.
> 
> Or we could use the new zone_allocator that you have added as the allocator
for
> the SnapshotWriter too and move your Zone zone(...) declaration above the
writer
> object.
> 
> Your choice.

Changed to use zone allocator in all tests involving reading into Dart_CMessage.