Decode the Dart message into a Dart_CMessage structure before calling the native port callback

Decode the Dart message into a Dart_CMessage structure before calling the native port callback

The native port callback is now passed the message as a decodes
Dart_CMessage structure. The Dart_CMessage structure is allocated
in a zone and the callback receiving it should expect the
lifetime to be controlled by the caller.

Added support for zones which do not require a current
isolate. Changed the GrowableArray to support allocating in
aprovided zone instead of the zone for the current isolate.

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

BUG=
TEST=

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

[siva, 2012-02-04 01:55:43]

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

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:565: struct Dart_CMessage;
Why not move the section
/** Message sending/receiving from native code */
over here that way you don't have to forward declare struct Dart_CMessage.

Also the native handlers etc. are related to sending/receiving messages from a
native thread.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1479: Dart_CObject* root;
I am beginning to wonder if we need "struct Dart_CMessage" and
instead use Dart_CObject* directly as a param to NativeMessageHandler.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/growable_arr...
File runtime/vm/growable_array.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/growable_arr...
runtime/vm/growable_array.h:29: explicit BaseGrowableArray(int initial_capacity,
Zone* zone = NULL)
BaseGrowableArray is not used anywhere but here, why not change the constructor
to take a Zone parameter instead of making it an optional parameter with a
default value.

We will have GrowableArray pass in the current zone of the Isolate or the zone
parameter it was passed in.

That way you would not need also these debug assertions and the isolate_zone_
variable etc.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/growable_arr...
runtime/vm/growable_array.h:137: : BaseGrowableArray<T,
ValueObject>(initial_capacity, zone) {}
Have two versions of the constructor, the one argument one which will invoke the
BaseGrowableArray constructor with the current zone of the isolate and the two
argument one which will pass in the zone parameter it got to the
BaseGrowableArray constructor.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:53: Zone zone;
Instead of modifying zone to accept a version without an Isolate why not use
ApiZone here. This is native code and should theoratically work with just the
dart_api stuff.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:58: &zone);
Do you really want a version of the reader with a Zone parameter, why not have
an allocator function which would use the Zone similar to the one in
ApiAllocator (dart_api_impl.cc) and it can access the zone wither from a static
variable if we have only one handler thread or use a thread local variable for
storing zone.

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

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/snapshot.h#n...
runtime/vm/snapshot.h:432: CMessageReader(const uint8_t* buffer, intptr_t
length, Zone* zone);
Ditto comment regarding this version of the constructor, do you really want
this,  can't we have an allocator that will do the same.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/snapshot.h#n...
runtime/vm/snapshot.h:476: Zone* zone_;
Ditto comment.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/zone.h#newco...
runtime/vm/zone.h:101: explicit Zone();
If you use ApiZone I guess these changes to Zone are not needed.

[Søren Gjesse, 2012-02-06 16:25:52]

Addressed some comments, however if we should use the ApiZone for allocation the
decoded message. I will have to change the GrowableArray to take an allocator
and have that ApiZone allocator being able to locate the ApiZone using a thread
local.

Is that the way forward?

Using ApiZone in a GrowableArray will not look good.

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

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:565: struct Dart_CMessage;
On 2012/02/04 01:55:43, asiva wrote:
> Why not move the section
> /** Message sending/receiving from native code */
> over here that way you don't have to forward declare struct Dart_CMessage.
> 
> Also the native handlers etc. are related to sending/receiving messages from a
> native thread.

Done.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1479: Dart_CObject* root;
On 2012/02/04 01:55:43, asiva wrote:
> I am beginning to wonder if we need "struct Dart_CMessage" and
> instead use Dart_CObject* directly as a param to NativeMessageHandler.

Mads and I also discussed this Friday. And as it does not add any value - except
for an additional indirection, so I have removed it and uses Dart_CObject
directly.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/growable_arr...
File runtime/vm/growable_array.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/growable_arr...
runtime/vm/growable_array.h:29: explicit BaseGrowableArray(int initial_capacity,
Zone* zone = NULL)
On 2012/02/04 01:55:43, asiva wrote:
> BaseGrowableArray is not used anywhere but here, why not change the
constructor
> to take a Zone parameter instead of making it an optional parameter with a
> default value.
> 
> We will have GrowableArray pass in the current zone of the Isolate or the zone
> parameter it was passed in.
> 
> That way you would not need also these debug assertions and the isolate_zone_
> variable etc.

Done.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/growable_arr...
runtime/vm/growable_array.h:137: : BaseGrowableArray<T,
ValueObject>(initial_capacity, zone) {}
On 2012/02/04 01:55:43, asiva wrote:
> Have two versions of the constructor, the one argument one which will invoke
the
> BaseGrowableArray constructor with the current zone of the isolate and the two
> argument one which will pass in the zone parameter it got to the
> BaseGrowableArray constructor.

Done.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:53: Zone zone;
On 2012/02/04 01:55:43, asiva wrote:
> Instead of modifying zone to accept a version without an Isolate why not use
> ApiZone here. This is native code and should theoratically work with just the
> dart_api stuff.

The problem with using an ApiZone here is that the passed-in zone is also used
for the allocation of the GrowableArray used for backwards references by the
CMessageReader.

It makes sense to use the ApiZone here as this callback is part of the API.
However I would then have to modify the GrowableArray to take an allocator
function instead of a zone - which might be the right thing to do. I will try to
see how that looks.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:58: &zone);
On 2012/02/04 01:55:43, asiva wrote:
> Do you really want a version of the reader with a Zone parameter, why not have
> an allocator function which would use the Zone similar to the one in
> ApiAllocator (dart_api_impl.cc) and it can access the zone wither from a
static
> variable if we have only one handler thread or use a thread local variable for
> storing zone.

We do need a thread local if we are not passing in a zone. I have prepared CL
http://codereview.chromium.org/9328042/ to make thread locals platform
independent. I will then add a thread local key for the NativeMessageHandler and
pass in an allocator which uses the thread local for finding the zone.

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

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/snapshot.h#n...
runtime/vm/snapshot.h:432: CMessageReader(const uint8_t* buffer, intptr_t
length, Zone* zone);
On 2012/02/04 01:55:43, asiva wrote:
> Ditto comment regarding this version of the constructor, do you really want
> this,  can't we have an allocator that will do the same.

See comment in native_message_handler.cc

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/zone.h#newco...
runtime/vm/zone.h:101: explicit Zone();
On 2012/02/04 01:55:43, asiva wrote:
> If you use ApiZone I guess these changes to Zone are not needed.

See previous comments.

[siva, 2012-02-07 01:03:23]

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:53: Zone zone;
You could make BaseGrowableArray use BaseZone* as the field in it instead of
Zone* and create a ApiGrowableArray structure in dart_api_state.h which passes
in the appropriate BaseZone from ApiZone got from the thread local storage.

If you think ApiGrowableArray won't be used a lot in the native code you could
pull the CMessageReader and CMessageWriter structure into this file and make
ApiGrowableArray a private class here.


On 2012/02/06 16:25:52, Søren Gjesse wrote:
> On 2012/02/04 01:55:43, asiva wrote:
> > Instead of modifying zone to accept a version without an Isolate why not use
> > ApiZone here. This is native code and should theoratically work with just
the
> > dart_api stuff.
> 
> The problem with using an ApiZone here is that the passed-in zone is also used
> for the allocation of the GrowableArray used for backwards references by the
> CMessageReader.
> 
> It makes sense to use the ApiZone here as this callback is part of the API.
> However I would then have to modify the GrowableArray to take an allocator
> function instead of a zone - which might be the right thing to do. I will try
to
> see how that looks.

https://chromiumcodereview.appspot.com/9325022/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:58: &zone);
I have reviewed that change, it seems fine to abstract out the thread local
stuff into platform as you might at some time want to use thread local data in
your thread pool implementation also.

On 2012/02/06 16:25:52, Søren Gjesse wrote:
> On 2012/02/04 01:55:43, asiva wrote:
> > Do you really want a version of the reader with a Zone parameter, why not
have
> > an allocator function which would use the Zone similar to the one in
> > ApiAllocator (dart_api_impl.cc) and it can access the zone wither from a
> static
> > variable if we have only one handler thread or use a thread local variable
for
> > storing zone.
> 
> We do need a thread local if we are not passing in a zone. I have prepared CL
> http://codereview.chromium.org/9328042/ to make thread locals platform
> independent. I will then add a thread local key for the NativeMessageHandler
and
> pass in an allocator which uses the thread local for finding the zone.

[Søren Gjesse, 2012-02-07 14:04:23]

Siva, thank you for the suggestions.

I changed BaseGrowableArray to take a BaseZone pointer for its
allocation, and added ApiGrowableArray as you suggested. It does
the trick, but I had to add some friend declarations to BaseZone.

In dart_api_state.h I added ApiNativeScope which holds an
ApiZone. The ApiNativeScope puts itself into a new thread local,
which is used to find the ApiZone. ApiGrowableArray will pick a
BaseZone from either the current isolate if there is not (that is
the case in many of the message tests) or from the current
ApiNativeScope. Currently the only place a ApiNativeScope is
created is when handling a received message in the native message
handler.

I have moved the CMessageReader to a separate set of files
dart_api_message.* and renamed to ApiMessageReader. I will move
the MessageWriter there as well in a separate change.

ptal.

[siva, 2012-02-09 00:45:29]

LGTM with comment regarding threads with an Isolate being allowed to use
ApiGrowableArray 
It would be good to restrict it to only the native message handlers first and
see if we ever get into a situation where we have to use it for threads which
have an Isolate.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart.cc#...
runtime/vm/dart.cc:38: ApiNativeScope::InitOnce();
Why don't you make this Api::InitOnce();

We have an Api class and all initialization related to the API can be done here,
in case we have some additional stuff to do in the future.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2444: void ApiNativeScope::InitOnce() {
Api::InitOnce() {
....
}

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
File runtime/vm/dart_api_message.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
runtime/vm/dart_api_message.h:50: Dart_CObject_Internal*
AsInternal(Dart_CObject* object) {
Looks like Dart_CObject_Internal is still defined in snapshot.h

Can that be moved here too?

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
File runtime/vm/dart_api_state.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
runtime/vm/dart_api_state.h:555: Isolate::Current() != NULL
Do you want to allow use of this data structure even for the case when an
isolate is present? Do you have a use case for that in the current code? I would
prefer if this was only used by code which does not have a current Isolate.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/growable...
File runtime/vm/growable_array.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/growable...
runtime/vm/growable_array.h:115: initial_capacity, zone->GetBaseZone()) {}
Are there any users for this variation of the constructor now, do we still need
it?

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/zone.h#n...
runtime/vm/zone.h:143: template<typename T> friend class ApiGrowableArray;
Why does ApiGrowableArray have to be a friend here?

[Søren Gjesse, 2012-02-09 08:44:01]

Thanks for the review.

I restricted ApiGrowableArray to only be able to use the zone from an active
ApiNativeScope.

It is still possible to have both a current isolate and an active
ApiNativeScope. Maybe we should also disallow ApiNativeScope when there is a
current isolate. The tests will have to be restructured then.

with comment regarding threads with an Isolate being allowed to use
 
It would be good to restrict it to only the native message handlers first and
see if we ever get into a situation where we have to use it for threads which
have an Isolate.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart.cc#...
runtime/vm/dart.cc:38: ApiNativeScope::InitOnce();
On 2012/02/09 00:45:29, asiva wrote:
> Why don't you make this Api::InitOnce();
> 
> We have an Api class and all initialization related to the API can be done
here,
> in case we have some additional stuff to do in the future.

Good point. I also moved the thread local key to the Api class so it might be
used more generally by the API if ever needed.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2444: void ApiNativeScope::InitOnce() {
On 2012/02/09 00:45:29, asiva wrote:
> Api::InitOnce() {
> ....
> }

Done.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
File runtime/vm/dart_api_message.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
runtime/vm/dart_api_message.h:50: Dart_CObject_Internal*
AsInternal(Dart_CObject* object) {
On 2012/02/09 00:45:29, asiva wrote:
> Looks like Dart_CObject_Internal is still defined in snapshot.h
> 
> Can that be moved here too?

Done.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
File runtime/vm/dart_api_state.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/dart_api...
runtime/vm/dart_api_state.h:555: Isolate::Current() != NULL
On 2012/02/09 00:45:29, asiva wrote:
> Do you want to allow use of this data structure even for the case when an
> isolate is present? Do you have a use case for that in the current code? I
would
> prefer if this was only used by code which does not have a current Isolate.

It was only used in the tests where the decoding is happening with a current
isolate.

I removed the support for ApiGrowableAarray using a current isolate and added
ApiNativeScope to the tests. We can have both a current isolate and an active
ApiNativeScope then. If that is not OK we should ensure that we do not enter a
ApiNativeScope with a current isolate and restructure the tests.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/growable...
File runtime/vm/growable_array.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/growable...
runtime/vm/growable_array.h:115: initial_capacity, zone->GetBaseZone()) {}
On 2012/02/09 00:45:29, asiva wrote:
> Are there any users for this variation of the constructor now, do we still
need
> it?

No, removed.

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://chromiumcodereview.appspot.com/9325022/diff/12001/runtime/vm/zone.h#n...
runtime/vm/zone.h:143: template<typename T> friend class ApiGrowableArray;
On 2012/02/09 00:45:29, asiva wrote:
> Why does ApiGrowableArray have to be a friend here?

It does not, removed.