Simplify and fix implicit closure check, speed up Closure_equals

Simplify and fix implicit closure check, speed up Closure_equals

This CL introduces a new Function kind, kImplicitClosureFunction, in
order to simplify check for implicit closures and make it more
efficient.

This CL also fixes Function::IsImplicitStaticClosureFunction(RawFunction*)
to correctly handle implicit closures created from static native
functions.

Closes #30203.

As the result of a faster check for implicit closures and slight
refactoring of Closure_equals, micro-benchmark exercising Closure_equals
speeds up from 9618ms to 6700ms for implicit closures case and
insignificantly for other cases.

R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/44e8da3ecd23de5fdd022d6de0d2ec895cc37beb

[alexmarkov, 2017-07-21 19:20:13]

alexmarkov@google.com changed reviewers:
+ asiva@google.com, zra@google.com

[zra, 2017-07-21 19:54:16]

https://codereview.chromium.org/2989493002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2989493002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:5993: RawFunction::Kind k = kind();
const

https://codereview.chromium.org/2989493002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6674: RawFunction* Function::NewClosureFunction(const
String& name,
Can these three methods use a common helper?

https://codereview.chromium.org/2989493002/diff/1/tests/isolate/message4_test...
File tests/isolate/message4_test.dart (right):

https://codereview.chromium.org/2989493002/diff/1/tests/isolate/message4_test...
tests/isolate/message4_test.dart:13: import
'package:async_helper/async_helper.dart';
package: imports should come after all dart: imports.

https://codereview.chromium.org/2989493002/diff/1/tests/isolate/message4_test...
tests/isolate/message4_test.dart:61: Isolate
Maybe instead something like:

main() async {
  ...
  Isolate i = await Isolate.spawn(echoMain, [initialReplyPort.sendPort,
testPort.sendPort]);
  SendPort ping = await initialReplyPort.first;
  runTests(ping, checks);
  Expect.isTrue(checks.length > 0);
  await completer.future;
  ping.send("halt");
  asyncEnd();
}

[alexmarkov, 2017-07-21 20:51:42]

https://codereview.chromium.org/2989493002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2989493002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:5993: RawFunction::Kind k = kind();
On 2017/07/21 19:54:16, zra wrote:
> const

Done.

https://codereview.chromium.org/2989493002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6674: RawFunction* Function::NewClosureFunction(const
String& name,
On 2017/07/21 19:54:16, zra wrote:
> Can these three methods use a common helper?

Done.

https://codereview.chromium.org/2989493002/diff/1/tests/isolate/message4_test...
File tests/isolate/message4_test.dart (right):

https://codereview.chromium.org/2989493002/diff/1/tests/isolate/message4_test...
tests/isolate/message4_test.dart:13: import
'package:async_helper/async_helper.dart';
On 2017/07/21 19:54:16, zra wrote:
> package: imports should come after all dart: imports.

Done.

https://codereview.chromium.org/2989493002/diff/1/tests/isolate/message4_test...
tests/isolate/message4_test.dart:61: Isolate
On 2017/07/21 19:54:16, zra wrote:
> Maybe instead something like:
> 
> main() async {
>   ...
>   Isolate i = await Isolate.spawn(echoMain, [initialReplyPort.sendPort,
> testPort.sendPort]);
>   SendPort ping = await initialReplyPort.first;
>   runTests(ping, checks);
>   Expect.isTrue(checks.length > 0);
>   await completer.future;
>   ping.send("halt");
>   asyncEnd();
> }

Done.

[zra, 2017-07-21 20:58:09]

lgtm w/ helper function rename.

https://codereview.chromium.org/2989493002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2989493002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2888: static RawFunction*
NewClosureFunction(RawFunction::Kind kind,
We try to avoid overloading. Maybe NewClosureFunctionHelper.

[alexmarkov, 2017-07-21 21:16:08]

https://codereview.chromium.org/2989493002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2989493002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2888: static RawFunction*
NewClosureFunction(RawFunction::Kind kind,
On 2017/07/21 20:58:09, zra wrote:
> We try to avoid overloading. Maybe NewClosureFunctionHelper.

Done. Renamed to NewClosureFunctionWithKind.

[alexmarkov, 2017-07-21 21:27:00]

Description was changed from

==========
Simplify and fix implicit closure check, speed up Closure_equals

This CL introduces a new Function kind, kImplicitClosureFunction, in
order to simplify check for implicit closures and make it more
efficient.

This CL also fixes Function::IsImplicitStaticClosureFunction(RawFunction*)
to correctly handle implicit closures created from static native
functions.

Closes #30203.

As the result of a faster check for implicit closures and slight
refactoring of Closure_equals, micro-benchmark exercising Closure_equals
speeds up from 9618ms to 6700ms for implicit closures case and
insignificantly for other cases.
==========

to

==========
Simplify and fix implicit closure check, speed up Closure_equals

This CL introduces a new Function kind, kImplicitClosureFunction, in
order to simplify check for implicit closures and make it more
efficient.

This CL also fixes Function::IsImplicitStaticClosureFunction(RawFunction*)
to correctly handle implicit closures created from static native
functions.

Closes #30203.

As the result of a faster check for implicit closures and slight
refactoring of Closure_equals, micro-benchmark exercising Closure_equals
speeds up from 9618ms to 6700ms for implicit closures case and
insignificantly for other cases.

R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/44e8da3ecd23de5fdd022d6de0d2ec895cc37beb
==========

[alexmarkov, 2017-07-21 21:27:02]

Committed patchset #3 (id:40001) manually as
44e8da3ecd23de5fdd022d6de0d2ec895cc37beb (presubmit successful).

[siva, 2017-07-21 21:47:17]

DBC

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc...
runtime/lib/function.cc:45: const Function& func_a = Function::Handle(zone,
receiver.function());
It probably makes sense to avoid creation of this handle
if other.IsClosure() is false.
The old code was doing that.

[alexmarkov, 2017-07-21 22:02:43]

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc...
runtime/lib/function.cc:45: const Function& func_a = Function::Handle(zone,
receiver.function());
On 2017/07/21 21:47:17, siva wrote:
> It probably makes sense to avoid creation of this handle
> if other.IsClosure() is false.
> The old code was doing that.

My gut feeling tells me that comparing a closure with non-closure is more rare
than comparing 2 closures. By moving IsClosure() check after
IsImplicitClosureFunction() I optimized cases of explicit and implicit static
closures (identity comparison is sufficient for them).

Do you think my assumption is wrong and comparison of closure with non-closure
happens more frequently than comparison of 2 closures?

[siva, 2017-07-21 22:51:25]

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc...
runtime/lib/function.cc:45: const Function& func_a = Function::Handle(zone,
receiver.function());
On 2017/07/21 22:02:43, alexmarkov wrote:
> On 2017/07/21 21:47:17, siva wrote:
> > It probably makes sense to avoid creation of this handle
> > if other.IsClosure() is false.
> > The old code was doing that.
> 
> My gut feeling tells me that comparing a closure with non-closure is more rare
> than comparing 2 closures. By moving IsClosure() check after
> IsImplicitClosureFunction() I optimized cases of explicit and implicit static
> closures (identity comparison is sufficient for them).
> 
> Do you think my assumption is wrong and comparison of closure with non-closure
> happens more frequently than comparison of 2 closures?

Not sure depends on the program being run, you should be able to figure that out
by running some flutter programs (posse, gallery etc.). 

other.IsClosure() is a very cheap function anyway (a virtual call)

[alexmarkov, 2017-07-24 16:11:47]

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/2989493002/diff/40001/runtime/lib/function.cc...
runtime/lib/function.cc:45: const Function& func_a = Function::Handle(zone,
receiver.function());
On 2017/07/21 22:51:24, siva wrote:
> On 2017/07/21 22:02:43, alexmarkov wrote:
> > On 2017/07/21 21:47:17, siva wrote:
> > > It probably makes sense to avoid creation of this handle
> > > if other.IsClosure() is false.
> > > The old code was doing that.
> > 
> > My gut feeling tells me that comparing a closure with non-closure is more
rare
> > than comparing 2 closures. By moving IsClosure() check after
> > IsImplicitClosureFunction() I optimized cases of explicit and implicit
static
> > closures (identity comparison is sufficient for them).
> > 
> > Do you think my assumption is wrong and comparison of closure with
non-closure
> > happens more frequently than comparison of 2 closures?
> 
> Not sure depends on the program being run, you should be able to figure that
out
> by running some flutter programs (posse, gallery etc.). 
> 
> other.IsClosure() is a very cheap function anyway (a virtual call)

Reverted to the earlier testing other.IsClosure(), in the next review
(https://codereview.chromium.org/2987703002/).