Use prototype inheritance to reduce number of is check properties.

Use prototype inheritance to reduce number of is check properties.

BUG=
R=floitsch@google.com

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

[herhut, 2015-01-13 14:18:04]

herhut@google.com changed reviewers:
+ karlklose@google.com

[floitsch, 2015-01-13 17:23:34]

LGTM if you can convince me that the is-check field is always on the super
class.

https://codereview.chromium.org/851473002/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/type_test_generator.dart (right):

https://codereview.chromium.org/851473002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:62:
!compiler.world.isSubclassOf(classElement, other)) {
Are you sure that [other] has the 'is-check' set?
Can you assert it?

[herhut, 2015-01-14 10:28:17]

https://codereview.chromium.org/851473002/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/type_test_generator.dart (right):

https://codereview.chromium.org/851473002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:62:
!compiler.world.isSubclassOf(classElement, other)) {
On 2015/01/13 17:23:34, floitsch wrote:
> Are you sure that [other] has the 'is-check' set?
> Can you assert it?

The general entry point, generateIsTests, is called for every class that is
emitted. It uses [emmiterTask.typeTestRegistry.checkedClasses] to decide,
whether a class needs support for is tests. As there is a single source of truth
for all classes, if a subclass would get the test, so will the superclass.

For the tests generated due to interfaces a class implements, the superclass
test will return false anyway.

An assert would be difficult, as we do not track what tests have been emitted
for a class. Also I think it is not required.

[floitsch, 2015-01-14 10:57:41]

https://codereview.chromium.org/851473002/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/type_test_generator.dart (right):

https://codereview.chromium.org/851473002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:62:
!compiler.world.isSubclassOf(classElement, other)) {
On 2015/01/14 10:28:17, herhut wrote:
> On 2015/01/13 17:23:34, floitsch wrote:
> > Are you sure that [other] has the 'is-check' set?
> > Can you assert it?
> 
> The general entry point, generateIsTests, is called for every class that is
> emitted. It uses [emmiterTask.typeTestRegistry.checkedClasses] to decide,
> whether a class needs support for is tests. As there is a single source of
truth
> for all classes, if a subclass would get the test, so will the superclass.
> 
> For the tests generated due to interfaces a class implements, the superclass
> test will return false anyway.
> 
> An assert would be difficult, as we do not track what tests have been emitted
> for a class. Also I think it is not required.

I'm not yet convinced that checkedClasses is the only reason we add is-checks to
classes.
For example backend.rti.requiredChecks (line 148) adds is-checks (after your
change through generateTypeCheck), and I wouldn't be sure that these classes are
in the checkedClasses list.

[herhut, 2015-01-26 15:22:01]

PTAL.

I now generate is checks for all classes dynamically. Not only is this smaller,
it also solves the issue of is checks for classes missing when they are only
needed for type tests.

[floitsch, 2015-01-26 16:19:29]

LGTM with comments.

Please make sure it works in minified code.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:363: str += name + "."
+ #fieldNamesProperty + "=[" + fieldNames + "];\n";
long line.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:437: // TODO(21896):
this test shouldn't be necessary. Without it
you should rebase. That TODO has been fixed some time ago.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:445:
prototype.$isObject = 1;
Are you sure this works? In minified code the name of the object class might not
be "Object".

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:445:
prototype.$isObject = 1;
I think we discussed it, but I changed my mind: this should be a function, since
it is faster to check for JS engines. Same below.

This is the code and documentation that is in js_array.dart for markFixedList:

===
    // Functions are stored in the hidden class and not as properties in
    // the object. We never actually look at the value, but only want
    // to know if the property exists.
    JS('void', r'#.fixed$length = Array', list);
===

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:456: object["$is" +
constructor.name] = 1;
Don't just use a magic prefix.
I believe that the namer even has the prefix as property.  (same for the
$isObject above).

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/type_test_generator.dart (right):

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:61: /// anyway. This
assumes that for every class an is-tests is generated
-anyway-.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:62: /// dyanically at
runtime anyway. We also always generate tests against
dynamically.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:68:
result.properties[namer.operatorIs(other)] = js('1');
probably ok to have the 1 here instead of a function (since it would otherwise
blow up the code).

[floitsch, 2015-01-26 16:47:46]

On 2015/01/26 16:19:29, floitsch wrote:
> LGTM with comments.
> 
> Please make sure it works in minified code.
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:363: str += name +
"."
> + #fieldNamesProperty + "=[" + fieldNames + "];\n";
> long line.
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:437: // TODO(21896):
> this test shouldn't be necessary. Without it
> you should rebase. That TODO has been fixed some time ago.
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:445:
> prototype.$isObject = 1;
> Are you sure this works? In minified code the name of the object class might
not
> be "Object".
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:445:
> prototype.$isObject = 1;
> I think we discussed it, but I changed my mind: this should be a function,
since
> it is faster to check for JS engines. Same below.
> 
> This is the code and documentation that is in js_array.dart for markFixedList:
> 
> ===
>     // Functions are stored in the hidden class and not as properties in
>     // the object. We never actually look at the value, but only want
>     // to know if the property exists.
>     JS('void', r'#.fixed$length = Array', list);
> ===
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:456: object["$is" +
> constructor.name] = 1;
> Don't just use a magic prefix.
> I believe that the namer even has the prefix as property.  (same for the
> $isObject above).
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> File pkg/compiler/lib/src/js_emitter/type_test_generator.dart (right):
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/type_test_generator.dart:61: /// anyway. This
> assumes that for every class an is-tests is generated
> -anyway-.
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/type_test_generator.dart:62: /// dyanically at
> runtime anyway. We also always generate tests against
> dynamically.
> 
>
https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_emitter/type_test_generator.dart:68:
> result.properties[namer.operatorIs(other)] = js('1');
> probably ok to have the 1 here instead of a function (since it would otherwise
> blow up the code).

Also. Please update the new emitter (after you have rebased).

[herhut, 2015-01-27 11:37:20]

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:363: str += name + "."
+ #fieldNamesProperty + "=[" + fieldNames + "];\n";
On 2015/01/26 16:19:29, floitsch wrote:
> long line.

Oops. Done.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:437: // TODO(21896):
this test shouldn't be necessary. Without it
On 2015/01/26 16:19:29, floitsch wrote:
> you should rebase. That TODO has been fixed some time ago.

Done.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:445:
prototype.$isObject = 1;
On 2015/01/26 16:19:29, floitsch wrote:
> Are you sure this works? In minified code the name of the object class might
not
> be "Object".

Of course it does not :) This was a bit rushed yesterday. Thanks!

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:445:
prototype.$isObject = 1;
On 2015/01/26 16:19:29, floitsch wrote:
> I think we discussed it, but I changed my mind: this should be a function,
since
> it is faster to check for JS engines. Same below.
> 
> This is the code and documentation that is in js_array.dart for markFixedList:
> 
> ===
>     // Functions are stored in the hidden class and not as properties in
>     // the object. We never actually look at the value, but only want
>     // to know if the property exists.
>     JS('void', r'#.fixed$length = Array', list);
> ===

I use the constructor now, which should be a function.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:456: object["$is" +
constructor.name] = 1;
On 2015/01/26 16:19:29, floitsch wrote:
> Don't just use a magic prefix.
> I believe that the namer even has the prefix as property.  (same for the
> $isObject above).

Done.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/type_test_generator.dart (right):

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:61: /// anyway. This
assumes that for every class an is-tests is generated
On 2015/01/26 16:19:29, floitsch wrote:
> -anyway-.

Done.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:62: /// dyanically at
runtime anyway. We also always generate tests against
On 2015/01/26 16:19:29, floitsch wrote:
> dynamically.

Done.

https://codereview.chromium.org/851473002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/type_test_generator.dart:68:
result.properties[namer.operatorIs(other)] = js('1');
On 2015/01/26 16:19:29, floitsch wrote:
> probably ok to have the 1 here instead of a function (since it would otherwise
> blow up the code).

Acknowledged.

[herhut, 2015-01-28 08:54:52]

Committed patchset #4 (id:60001) manually as 43229 (presubmit successful).