Collect the types used in is-checks in the resolver phase.

Collect the types used in is-checks in the resolver phase.

This is the first step to reducing the amount of runtime type information, but it does not currently lead to less generated code. The next step is to do the same for the instantiated types and use both sets of types to calculate the set of instantiations that need type information.

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

[ngeoffray, 2012-08-30 11:03:33]

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/c...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/c...
lib/compiler/implementation/compiler.dart:347: // Register is-checks for all
special classes.
Why? Is checks are for emitting the is$ClassName method on user-defined classes,
so I don't think special classed need it.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/c...
lib/compiler/implementation/compiler.dart:842: registerIsCheck(Type type) {
Please remove this helper method, it is too confusing to have it there and in
enqueuer.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1044: compiler.registerIsCheck(type);
Just enqueuer.resolution

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1355: compiler.registerIsCheck(type);
ditto

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1619:
compiler.enqueuer.codegen.registerIsCheck(type);
Not codegen, just resolution.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1874: doInCheckContext(() =>
visitIn(node.type, blockScope));
Do you need the inCheckContext state? If visiting node.type returns a type you
don't need it.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/t...
File lib/compiler/implementation/typechecker.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/t...
lib/compiler/implementation/typechecker.dart:87: int hashCode() =>
stringName.hashCode();
I've been advised not to return the hashCode of a field in a hashCode method.
Multiplying it is better.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/t...
lib/compiler/implementation/typechecker.dart:213: return myParameters.isEmpty()
&& otherParameters.isEmpty();
This is the same code than line 145. Add an equals method in Link?

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/u...
File lib/compiler/implementation/universe.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/u...
lib/compiler/implementation/universe.dart:35: get checkedClasses {
What is that for? Why not using isChecks?

[karlklose, 2012-08-30 13:04:30]

PTAL.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/c...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/c...
lib/compiler/implementation/compiler.dart:347: // Register is-checks for all
special classes.
Done.

I added internalErrors when elements were not found in the isChecks set, where
they before this change have been added in the backend. Adding these was
necessary to not trigger these errors.

However, after removing this and the internalErrors in the backend, all tests
pass. So it seems that adding the isChecks in the backend was unnecessary.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/c...
lib/compiler/implementation/compiler.dart:842: registerIsCheck(Type type) {
On 2012/08/30 11:03:33, ngeoffray wrote:
> Please remove this helper method, it is too confusing to have it there and in
> enqueuer.

Done.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1044: compiler.registerIsCheck(type);
On 2012/08/30 11:03:33, ngeoffray wrote:
> Just enqueuer.resolution

Done.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1355: compiler.registerIsCheck(type);
On 2012/08/30 11:03:33, ngeoffray wrote:
> ditto

Done.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1619:
compiler.enqueuer.codegen.registerIsCheck(type);
On 2012/08/30 11:03:33, ngeoffray wrote:
> Not codegen, just resolution.

Done.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/r...
lib/compiler/implementation/resolver.dart:1874: doInCheckContext(() =>
visitIn(node.type, blockScope));
True, but currently ResolverVisitor is a visitor on Elements, so this would be
unsafe in checked mode.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/t...
File lib/compiler/implementation/typechecker.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/t...
lib/compiler/implementation/typechecker.dart:87: int hashCode() =>
stringName.hashCode();
Done, forgot this one.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/t...
lib/compiler/implementation/typechecker.dart:213: return myParameters.isEmpty()
&& otherParameters.isEmpty();
On 2012/08/30 11:03:33, ngeoffray wrote:
> This is the same code than line 145. Add an equals method in Link?

Done.

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/u...
File lib/compiler/implementation/universe.dart (right):

http://codereview.chromium.org/10911006/diff/11/lib/compiler/implementation/u...
lib/compiler/implementation/universe.dart:35: get checkedClasses {
Currently, the code emitter generates the is$-methods based on elements. Instead
of running through the isChecks multiple times to find out whether there is a
type with the matching element in the set, I create this set once.

[ngeoffray, 2012-08-30 13:17:46]

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:548:
enqueuer.codegen.universe.computeRequiredTypes(
Why is that here? The computation should be done on the codegen universe, and
once all methods have been compiled (ie just before we enter emitting).

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
File lib/compiler/implementation/js_backend/native_emitter.dart (right):

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
lib/compiler/implementation/js_backend/native_emitter.dart:386: for (Type type
in compiler.codegenWorld.isChecks) {
Shouldn't you use checkedClasses here also? otherwise you would create the is$
function multiple times.

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
File lib/compiler/implementation/universe.dart (right):

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
lib/compiler/implementation/universe.dart:35: // TODO(karlklose): the second
argument should be a  set of types, too.
The second argument is not used.

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
File lib/compiler/implementation/util/link_implementation.dart (right):

http://codereview.chromium.org/10911006/diff/7001/lib/compiler/implementation...
lib/compiler/implementation/util/link_implementation.dart:146: while
(!myElements.isEmpty() && !other.isEmpty()) {
How about doing it recursively?
if (other.isEmpty()) return false;
if (other.head != head) return false;
return other.tail == tail;

[karlklose, 2012-09-05 09:36:01]

PTAL.

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:548:
enqueuer.codegen.universe.computeRequiredTypes(
On 2012/08/30 13:17:46, ngeoffray wrote:
> Why is that here? The computation should be done on the codegen universe, and
> once all methods have been compiled (ie just before we enter emitting).

Done.

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/js_backend/native_emitter.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/native_emitter.dart:386: for (Type type
in compiler.codegenWorld.isChecks) {
On 2012/08/30 13:17:46, ngeoffray wrote:
> Shouldn't you use checkedClasses here also? otherwise you would create the is$
> function multiple times.

Done.

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:35: // TODO(karlklose): the second
argument should be a  set of types, too.
Done, removed.

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/util/link_implementation.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/7001/lib/compiler/implem...
lib/compiler/implementation/util/link_implementation.dart:146: while
(!myElements.isEmpty() && !other.isEmpty()) {
I could. Do you want me to?

[ngeoffray, 2012-09-05 12:50:40]

LGTM!

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1631: if
(compiler.enableTypeAssertions && type != null) {
use inCheckContext

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
lib/compiler/implementation/universe.dart:34: // TODO(karlklose): add the set of
instantiatedtypes as second argument.
Why is the set of instantiatedTypes needed? You want to remove classes that are
not in that set? Do you really care? The backend runs through the list of
instantiated classes anyway.

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/util/link_implementation.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
lib/compiler/implementation/util/link_implementation.dart:144: if (other is
!Link<T>) return false;
Making it recursive looks easier to understand. But feel free to disagree.

[karlklose, 2012-09-06 11:04:01]

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1631: if
(compiler.enableTypeAssertions && type != null) {
On 2012/09/05 12:50:40, ngeoffray wrote:
> use inCheckContext

Done.

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
lib/compiler/implementation/universe.dart:34: // TODO(karlklose): add the set of
instantiatedtypes as second argument.
It is important for checks on type arguments to know what type instantiations
can occur.

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/util/link_implementation.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
lib/compiler/implementation/util/link_implementation.dart:144: if (other is
!Link<T>) return false;
I will keep it as it is.

[ngeoffray, 2012-09-06 11:07:46]

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10911006/diff/2011/lib/compiler/implem...
lib/compiler/implementation/universe.dart:34: // TODO(karlklose): add the set of
instantiatedtypes as second argument.
On 2012/09/06 11:04:02, karlklose wrote:
> It is important for checks on type arguments to know what type instantiations
> can occur.

So you can just return false for them?

[karlklose, 2012-09-06 11:32:44]

Yes. Eventually we also want to know for which of the instantiated types we need
to create runtime type information (because there is a matching is-Check).

[ngeoffray, 2012-09-06 11:35:49]

On 2012/09/06 11:32:44, karlklose wrote:
> Yes. Eventually we also want to know for which of the instantiated types we
need
> to create runtime type information (because there is a matching is-Check).

Yes, but that's being covered by the isChecks set.

[floitsch, 2012-09-06 13:02:02]

LGTM.