Made simple instance-of checks fast for unoptimized code.

Made simple instance-of checks fast for unoptimized code.
This change makes dart2js 15% faster when compiling 60KLOC. 15sec -> 13sec.

BUG=
R=regis@google.com

Committed: https://github.com/dart-lang/sdk/commit/21a3a27aff2661b6e5987412c585eede76510e5c

[bakster, 2016-07-13 23:56:02]

bak@google.com changed reviewers:
+ regis@google.com

[bakster, 2016-07-13 23:56:03]

Please review my faster instance-of CL.
//Lars

[regis, 2016-07-14 01:41:46]

I'll review again as you asked me to, but I was almost done. So here are my
comments so far. Many are just about style. Feel free to ignore them.

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

https://codereview.chromium.org/2147123002/diff/1/runtime/lib/object.cc#newco...
runtime/lib/object.cc:171: // simpleInstanceOfType and it is a non-negative
test.
It would be interesting to do it for the negative case as well and see if it
helps dart2js.
I suppose a 'negate' extra argument would not work, but you would need a
separate call (e.g. Object_simpleNotInstanceOf).

https://codereview.chromium.org/2147123002/diff/1/runtime/lib/object_patch.dart
File runtime/lib/object_patch.dart (right):

https://codereview.chromium.org/2147123002/diff/1/runtime/lib/object_patch.da...
runtime/lib/object_patch.dart:64: bool _simpleInstanceOfFalse(type) => false;
The names are slightly confusing. I'd prefer:
_simpleInstanceOfReturnsTrue and _simpleInstanceOfReturnsFalse

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

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:795: Function& target
const Function& target

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1547: // Bail if the type is unknown at compile
time.
'unknown' is not quite the right term. Maybe 'still uninstantiated' at compile
time?

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1550: // Bail if the type is a function.
... a function type or Dart Function type.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1553: const Class& type_class =
Class::Handle(type.type_class());
Calling type_class() on an AbstractType is not always safe. You are supposed to
check first with HasResolvedTypeClass().
I think it works here, because problematic cases (e.g. ParameterType) have
already been eliminated above.
So you can leave it as is, or add an ASSERT(type.HasResolvedTypeClass());

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1554: // Bail if the type is any type
parameters.
is -> has
or better:
Bail if the class of the type declares any type parameters.

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

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:12851: bool ICData::ValidateInteceptor(const Function&
target) const {
...Interceptor(...

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:12854: || target.raw() ==
store->simple_instance_of_false_function());
We usually use extra parenthesis and place the || at the end of the line.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:12864: ASSERT(target.name() == target_name() ||
ValidateInteceptor(target));
parenthesis

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

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:2093: bool ValidateInteceptor(const Function& target) const;
ValidateInterceptor

[regis, 2016-07-14 17:05:27]

LGTM

[Florian Schneider, 2016-07-14 17:18:02]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2016-07-14 17:18:03]

dbc:

https://codereview.chromium.org/2147123002/diff/20001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/2147123002/diff/20001/runtime/vm/object_store...
runtime/vm/object_store.cc:96: compile_time_constants_(Array::null()),
Same here.

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

https://codereview.chromium.org/2147123002/diff/20001/runtime/vm/object_store...
runtime/vm/object_store.h:591: V(RawArray*, compile_time_constants_)            
                           \
This line shouldn't be here anymore. Incorrect merge?

[bakster, 2016-07-14 17:59:42]

Fixed

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

https://codereview.chromium.org/2147123002/diff/1/runtime/lib/object.cc#newco...
runtime/lib/object.cc:171: // simpleInstanceOfType and it is a non-negative
test.
On 2016/07/14 01:41:45, regis wrote:
> It would be interesting to do it for the negative case as well and see if it
> helps dart2js.
> I suppose a 'negate' extra argument would not work, but you would need a
> separate call (e.g. Object_simpleNotInstanceOf).

Yes, that is an easy change.
However, in the benchmarks I measured, it is hardly used.

https://codereview.chromium.org/2147123002/diff/1/runtime/lib/object_patch.dart
File runtime/lib/object_patch.dart (right):

https://codereview.chromium.org/2147123002/diff/1/runtime/lib/object_patch.da...
runtime/lib/object_patch.dart:64: bool _simpleInstanceOfFalse(type) => false;
On 2016/07/14 01:41:45, regis wrote:
> The names are slightly confusing. I'd prefer:
> _simpleInstanceOfReturnsTrue and _simpleInstanceOfReturnsFalse

I used that naming first but I ran into the 80 character limit so many time I
decided to change it to a shorter name.
If you feel strongly about it, I'll be happy to break some lines.
//Lars

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

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:795: Function& target
On 2016/07/14 01:41:45, regis wrote:
> const Function& target

Done.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1547: // Bail if the type is unknown at compile
time.
On 2016/07/14 01:41:45, regis wrote:
> 'unknown' is not quite the right term. Maybe 'still uninstantiated' at compile
> time?

Done.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1550: // Bail if the type is a function.
On 2016/07/14 01:41:45, regis wrote:
> ... a function type or Dart Function type.

Done.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1553: const Class& type_class =
Class::Handle(type.type_class());
On 2016/07/14 01:41:45, regis wrote:
> Calling type_class() on an AbstractType is not always safe. You are supposed
to
> check first with HasResolvedTypeClass().
> I think it works here, because problematic cases (e.g. ParameterType) have
> already been eliminated above.
> So you can leave it as is, or add an ASSERT(type.HasResolvedTypeClass());

I inserted the ASSERT.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1554: // Bail if the type is any type
parameters.
On 2016/07/14 01:41:45, regis wrote:
> is -> has
> or better:
> Bail if the class of the type declares any type parameters.

Done.

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

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:12851: bool ICData::ValidateInteceptor(const Function&
target) const {
On 2016/07/14 01:41:46, regis wrote:
> ...Interceptor(...

Done.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:12854: || target.raw() ==
store->simple_instance_of_false_function());
On 2016/07/14 01:41:46, regis wrote:
> We usually use extra parenthesis and place the || at the end of the line.

Done.

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:12864: ASSERT(target.name() == target_name() ||
ValidateInteceptor(target));
On 2016/07/14 01:41:46, regis wrote:
> parenthesis

Done.

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

https://codereview.chromium.org/2147123002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:2093: bool ValidateInteceptor(const Function& target) const;
On 2016/07/14 01:41:46, regis wrote:
> ValidateInterceptor

Done.

https://codereview.chromium.org/2147123002/diff/20001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/2147123002/diff/20001/runtime/vm/object_store...
runtime/vm/object_store.cc:96: compile_time_constants_(Array::null()),
On 2016/07/14 17:18:03, Florian Schneider wrote:
> Same here.

Done.

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

https://codereview.chromium.org/2147123002/diff/20001/runtime/vm/object_store...
runtime/vm/object_store.h:591: V(RawArray*, compile_time_constants_)            
                           \
On 2016/07/14 17:18:03, Florian Schneider wrote:
> This line shouldn't be here anymore. Incorrect merge?

Done.

[regis, 2016-07-14 18:08:37]

Still LGTM

[bakster, 2016-07-14 18:11:22]

Description was changed from

==========
Made simple instance-of checks fast for unoptimized code.
This change makes dart2js 15% faster when compiling 60KLOC. 15sec -> 13sec.

BUG=
==========

to

==========
Made simple instance-of checks fast for unoptimized code.
This change makes dart2js 15% faster when compiling 60KLOC. 15sec -> 13sec.

BUG=
R=regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/21a3a27aff2661b6e5987412c585eede76510e5c
==========

[bakster, 2016-07-14 18:11:23]

Committed patchset #3 (id:40001) manually as
21a3a27aff2661b6e5987412c585eede76510e5c (presubmit successful).