Performance improvement for the Dart VM runtime method

Performance improvement for the Dart VM runtime method 
Dart_ExternalStringGetPeer. This change is basically an unrolling 
of the templates that previously defined the method, to reduce 
the number of handles allocated. 

This change is supported by the DartStringAccess VM benchmark, which 
showed a ~20x improvement from the above change. Currently this 
benchmark focuses on Dart_ExternalStringGetPeer, rather than String 
performance in general. 

Please review this carefully. Commented-out is a "NoGCScope no_gc_scope;" 
line, which when enabled asserts that GC doesn't happen when this function 
is called. However, one of the tests fails this check, though when I 
set a breakpoint in gdb before the NoGCScope item is allocated, it's not 
hit before the assertion fails. I commented out that line, in the hope 
that more senior reviewers suggest improvements.
Committed: https://code.google.com/p/dart/source/detail?r=11534

[Tom Ball, 2012-08-28 21:42:59]

Take 2: dropped previous CL, created new one with correct changes, verified
diffs before sending this out.  I'm sorry for the previous disaster.

[Ivan Posva, 2012-08-28 22:21:20]

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/benchmark_t...
File runtime/vm/benchmark_test.cc (right):

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/benchmark_t...
runtime/vm/benchmark_test.cc:234: for (int64_t i = 0; i < iterations; i++) {
Since this is essentially the benchmark you can drop all of the surrounding
machinery (creating scripts, native functions, Dart invocations) and run this
loop directly.

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1535: if (Dart_IsExternalString(object)) {
I would restructure this a bit to avoid duplicate work and to avoid having part
of the regularly expected work be in the default clause.

NoGCScope no_gc_scope;
intptr_t class_id = Api::ClassId(object);
RawObject* raw_obj = Api::UnwrapHandle(object);
switch (class_id) {
  case kExternalOneByteString:
    ...
    *peer = ...
    break;
  case kExternalTwoByteString:
    ...
    break;
  case kExternalFourByteString:
    ...
    break;
  default: {
    // Error handling is needed.
    if (!RawObject::IsStringClassId(class_id)) {
      RETURN_TYPE_ERROR(...);
    } else {
      ...
    }
}
return Api::Success(Isolate::Current());

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1542:
(*(reinterpret_cast<RawExternalOneByteString**>(object)))->ptr();
Please do not reach into the internals of handles. There is a convenience method
for this: Api::UnwrapHandle
By doing this we can change the internal implementation or run extra
verification in debug builds.

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1544: raw_peer = data != NULL ? data->peer() : NULL;
This extra check for data being NULL is not necessary.

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/raw_object....
runtime/vm/raw_object.h:1137: friend Dart_Handle
Dart_ExternalStringGetPeer(Dart_Handle o, void**);
Please remove the o to be consistent.
Also can you avoid the above block declaration of the functions being friended?

[Tom Ball, 2012-08-29 00:16:39]

Thanks for the suggestions, the code is much easier to read.  Two issues remain:

. NoGCScope fails one of the VM tests, and
. I don't know how to declare the friend statements in raw_object.h, so that an
extern "C" declaration block isn't necessary.

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1535: if (Dart_IsExternalString(object)) {
Thanks, this looks much cleaner -- done.

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1542:
(*(reinterpret_cast<RawExternalOneByteString**>(object)))->ptr();
On 2012/08/28 22:21:20, Ivan Posva wrote:
> Please do not reach into the internals of handles. There is a convenience
method
> for this: Api::UnwrapHandle
> By doing this we can change the internal implementation or run extra
> verification in debug builds.

Done.

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1544: raw_peer = data != NULL ? data->peer() : NULL;
On 2012/08/28 22:21:20, Ivan Posva wrote:
> This extra check for data being NULL is not necessary.

Done.

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/10897014/diff/6/runtime/vm/raw_object....
runtime/vm/raw_object.h:1137: friend Dart_Handle
Dart_ExternalStringGetPeer(Dart_Handle o, void**);
The 'o' is removed, but I think the block declaration is needed, because
otherwise the compiler reports:

error: previous declaration of ‘_Dart_Handle*
dart::Dart_ExternalStringGetPeer(Dart_Handle, void**)’ with ‘C++’ linkage

I tried removing the block and using ::Dart_ExternalStringGetPeer(...) in the
friend statement, since the classes are in the dart namespace but the API
functions aren't, but that failed with:


../runtime/vm/raw_object.h:1129:32: error: ‘dart::ExternalStringData<unsigned
char>* dart::RawExternalOneByteString::external_data_’ is private
../runtime/vm/dart_api_impl.cc:1541:55: error: within this context

Any ideas?

[cshapiro, 2012-08-29 00:39:02]

Hi Tom,

Can we take a look at the NoGCScope issue together tomorrow?  I have a theory
about that but I would like to test it in the debugger with you.

Carl

[Tom Ball, 2012-08-29 00:45:15]

Thanks for the offer, Carl, I'll take you up on it tomorrow.

[Ivan Posva, 2012-08-29 05:59:30]

LGTM with comments and once the NoGCScope issue is identified.

-Ivan

https://chromiumcodereview.appspot.com/10897014/diff/9001/runtime/vm/benchmar...
File runtime/vm/benchmark_test.cc (right):

https://chromiumcodereview.appspot.com/10897014/diff/9001/runtime/vm/benchmar...
runtime/vm/benchmark_test.cc:229: // 80% of benchmark is spent in the following
call.
Is the 80% value still correct here?

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

https://chromiumcodereview.appspot.com/10897014/diff/9001/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:1542: *peer = data->peer();
How about this?
RawExternalOneByteString* raw_string =
    reinterpret_cast<RawExternalOneByteString*(raw_obj);
*peer = raw_string->ptr()->external_data_->peer();

https://chromiumcodereview.appspot.com/10897014/diff/9001/runtime/vm/raw_obje...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/10897014/diff/9001/runtime/vm/raw_obje...
runtime/vm/raw_object.h:1137: friend Dart_Handle
Dart_ExternalStringGetPeer(Dart_Handle, void**);
Have you tried this?
friend DART_EXTERN_C Dart_Handle Dart_ExternalStringGetPeer(Dart_Handle,
void**);

[turnidge, 2012-08-29 16:53:35]

DBC ("Drive-By Comment") -turnidge

http://codereview.chromium.org/10897014/diff/9001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/10897014/diff/9001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1129: Dart_Handle
Dart_ExternalStringGetPeer(Dart_Handle, void**);
Does this need to be listed x3?

[Tom Ball, 2012-08-29 17:20:40]

http://codereview.chromium.org/10897014/diff/9001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/10897014/diff/9001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1129: Dart_Handle
Dart_ExternalStringGetPeer(Dart_Handle, void**);
I really, really, really, want it declared? ;-)

It's 1x now, thanks.

http://codereview.chromium.org/10897014/diff/9001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1137: friend Dart_Handle
Dart_ExternalStringGetPeer(Dart_Handle, void**);
That fails with a syntax error.  According to
http://bytes.com/topic/c/answers/127397-what-proper-syntax-extern-c-friend-vo...,
an explicit extern "C" declaration is required.  Thanks to Todd's review,
though, that declaration is less verbose now.