Keep a persistent handle for the bindings classes to avoid repeated lookups that involve performing…

Keep a persistent handle for the bindings classes to avoid repeated lookups that involve performing more API calls and allocating Dart strings.

R=asiva@google.com

Committed: https://src.chromium.org/viewvc/multivm?view=rev&revision=1376

[rmacnak, 2013-09-09 21:57:01]

Statically assign indices for the binding classes.

[siva, 2013-09-10 00:18:50]

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
File Source/bindings/dart/DartDOMWrapper.cpp (right):

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.cpp:68:
map->append(reinterpret_cast<Dart_PersistentHandle>(0));
Might be more efficient to do:

if (map->size() <= index) {
    intptr_t begin = map->size();
    map->expandCapacity(index + 1);
    for (intptr_t i = begin; i <= index; i++) {
        map->at(i) = reinterpret_cast<Dart_PersistentHandle>(0);
    }
}

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.cpp:82: map->insert(index, persistentType);
insert here in the implementation of Vector seems to insert this element at the
index moving all elements out by 1. This will cause errors as previously added
elements would get shifted.

I wonder if you are supposed to do
map->at(index) = persistentType;

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.h:184: static intptr_t nextClassIndex;
Not used anymore right, could be deleted.

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.h:195: return false;
Seems to assert and check, I think the ASSERT is not needed.

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.h:200: static Dart_Handle dartClass()
This can also be private right.

[rmacnak, 2013-09-10 20:09:50]

Now using the cache when instantiating wrappers. Assigned class ids for the 3/4
classes explicitly passing strings to toDartWithClassName. Changed kBindingCid
to dartClassId to conform with Blink's style. Tests pass!

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
File Source/bindings/dart/DartDOMWrapper.cpp (right):

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.cpp:68:
map->append(reinterpret_cast<Dart_PersistentHandle>(0));
On 2013/09/10 00:18:50, siva wrote:
> Might be more efficient to do:
> 
> if (map->size() <= index) {
>     intptr_t begin = map->size();
>     map->expandCapacity(index + 1);
>     for (intptr_t i = begin; i <= index; i++) {
>         map->at(i) = reinterpret_cast<Dart_PersistentHandle>(0);
>     }
> }

expandCapacity is private, but grow works

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.cpp:82: map->insert(index, persistentType);
On 2013/09/10 00:18:50, siva wrote:
> insert here in the implementation of Vector seems to insert this element at
the
> index moving all elements out by 1. This will cause errors as previously added
> elements would get shifted.
> 
> I wonder if you are supposed to do
> map->at(index) = persistentType;

map->data()[index] = persistentType seems to do the right thing

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.h:184: static intptr_t nextClassIndex;
On 2013/09/10 00:18:50, siva wrote:
> Not used anymore right, could be deleted.

Removed.

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.h:195: return false;
On 2013/09/10 00:18:50, siva wrote:
> Seems to assert and check, I think the ASSERT is not needed.

Removed.

https://chromiumcodereview.appspot.com/23895004/diff/4001/Source/bindings/dar...
Source/bindings/dart/DartDOMWrapper.h:200: static Dart_Handle dartClass()
On 2013/09/10 00:18:50, siva wrote:
> This can also be private right.

Hidden.

[siva, 2013-09-12 18:33:33]

lgtm

https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
File Source/bindings/dart/DartDOMData.h (right):

https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
Source/bindings/dart/DartDOMData.h:68: typedef Dart_PersistentHandle
ClassTable[NumClassIds];
kNumWebkitClassIds

https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
File Source/bindings/dart/DartDOMWrapper.cpp (right):

https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
Source/bindings/dart/DartDOMWrapper.cpp:51: Dart_PersistentHandle
persistentType;
ASSERT(index < kNumWebKitClassIds);

https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
Source/bindings/dart/DartDOMWrapper.cpp:65: }
Maybe structure this code as:

persistentType = (*table)[index];
if (persistentType) {
  type = Dart_handleFromPersistent(persistentType);
} else {
  .....
  .....
}
return type;

[rmacnak, 2013-09-12 22:02:04]

On 2013/09/12 18:33:33, siva wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
> File Source/bindings/dart/DartDOMData.h (right):
> 
>
https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
> Source/bindings/dart/DartDOMData.h:68: typedef Dart_PersistentHandle
> ClassTable[NumClassIds];
> kNumWebkitClassIds
> 
>
https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
> File Source/bindings/dart/DartDOMWrapper.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
> Source/bindings/dart/DartDOMWrapper.cpp:51: Dart_PersistentHandle
> persistentType;
> ASSERT(index < kNumWebKitClassIds);

Done.

>
https://chromiumcodereview.appspot.com/23895004/diff/22001/Source/bindings/da...
> Source/bindings/dart/DartDOMWrapper.cpp:65: }
> Maybe structure this code as:
> 
> persistentType = (*table)[index];
> if (persistentType) {
>   type = Dart_handleFromPersistent(persistentType);
> } else {
>   .....
>   .....
> }
> return type;

Done.

[rmacnak, 2013-09-12 22:02:42]

Committed patchset #7 manually as r1376 (presubmit successful).