Stop resolving all of js_helper unconditionally.

Stop resolving all of js_helper unconditionally.
Committed: https://code.google.com/p/dart/source/detail?r=18617

[ngeoffray, 2013-02-18 10:19:48]

I got tired of seeing these generally irrelevant helpers being resolved just
because they are in js_helper. They make resolution slow, and type inference and
global optimizations less effective.

Some numbers of interest collected on my machine:

# test.py --checked --host-checked
Before: 17:23s
After: 16:45s

# test.py
Before: 7.27s
After: 7.02s

[kasperl, 2013-02-18 10:31:52]

LGTM.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:890: ||cls ==
compiler.stringClass) {
||cls -> cls

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:954: //
expression in an if/while/for is a boolean. TODO(ngeoffray):
Put the TODO on the next line in a "fresh" comment.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:999: if
(compiler.enableTypeAssertions) {
Add a comment that explains why we need to register different things for checked
mode is checks.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1068:
compiler.enqueuer.codegen.registerStaticUse(getCyclicThrowHelper());
Use registerLazyField instead of all this and remove the comment about the
cyclic-error helper?

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:389: //
TODO(ngeoffray): We could do better here.
Improve comment to explain how.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2101:
compiler.enqueuer.resolution.registerIsCheck(type);
Why are we unconditionally registering the is check here? If it's because the as
check needs it then maybe the registerAsCheck should take care of that?

[karlklose, 2013-02-18 10:35:52]

LGTM.

It would be nice if we could encapsulate the registration of js_helpers in
helper functions in the resolver.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1653:
compiler.backend.registerThrowNoSuchMethod();
Can you move this to warnAndCreateErroneousElement?

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1955:
compiler.backend.registerThrowNoSuchMethod();
Ditto.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1976:
compiler.backend.registerThrowNoSuchMethod();
Ditto.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2087:
compiler.backend.registerThrowNoSuchMethod();
Ditto.

[ngeoffray, 2013-02-18 11:32:18]

Thanks Karl and Kasper

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:890: ||cls ==
compiler.stringClass) {
On 2013/02/18 10:31:52, kasperl wrote:
> ||cls -> cls

Done.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:954: //
expression in an if/while/for is a boolean. TODO(ngeoffray):
On 2013/02/18 10:31:52, kasperl wrote:
> Put the TODO on the next line in a "fresh" comment.

Done.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:999: if
(compiler.enableTypeAssertions) {
On 2013/02/18 10:31:52, kasperl wrote:
> Add a comment that explains why we need to register different things for
checked
> mode is checks.

Done.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1068:
compiler.enqueuer.codegen.registerStaticUse(getCyclicThrowHelper());
On 2013/02/18 10:31:52, kasperl wrote:
> Use registerLazyField instead of all this and remove the comment about the
> cyclic-error helper?

This is at the codegen level, so I cannot call registerLazyField which is at the
resolution level.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:389: //
TODO(ngeoffray): We could do better here.
On 2013/02/18 10:31:52, kasperl wrote:
> Improve comment to explain how.

Done.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1653:
compiler.backend.registerThrowNoSuchMethod();
On 2013/02/18 10:35:52, karlklose wrote:
> Can you move this to warnAndCreateErroneousElement?

Actually, I'd rather not. warnAndCreateErroneousElement seems like a generic
helper that could be used for non-no such method related things. Keeping the
registration of the helper close to where we found the problem avoids the
possibility of registering the helper when it's actually not needed.

https://codereview.chromium.org/12299008/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2101:
compiler.enqueuer.resolution.registerIsCheck(type);
On 2013/02/18 10:31:52, kasperl wrote:
> Why are we unconditionally registering the is check here? If it's because the
as
> check needs it then maybe the registerAsCheck should take care of that?

Done.

[ahe, 2013-02-18 12:38:04]

Nice, LGTM!

But you forgot to add a test for the TODO you fixed.

Cheers,
Peter

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:130: // helper methods.
Categories like this doesn't work, what happens if a method is added after them?

I would prefer if you could add real documentation comments to each of the
methods.

For example:

/// Called during resolution (and SSA graph building?) to record that [cls] was
instantiated from [element].
void registerInstantiatedClass(Element user, ClassElement cls, Enqueuer world)
{}

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:132: void
registerStringInterpolation() {}
I think all of the following methods should take the element which add the
dependency. This will help us shortly when we start looking at dynamic
recompilation.

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1313:
Map<String, SourceString> nativeNames = const <String, SourceString> {
Document this.

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1324:
Map<String, SourceString> castNames = const <String, SourceString> {
Ditto.

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2098: if
(operatorString == 'is' || operatorString == 'as') {
This has to be identical.

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2102: if
(operatorString == 'as') {
Identical.

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2396:
warnArgumentMismatch(node.send, constructor);
I would prefer if you did this change separately and tested it.

https://codereview.chromium.org/12299008/diff/3010/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2773: if
(!node.formals.nodes.tail.tail.isEmpty) {
Generally, I really like going from && to nested ifs. It is just more readable
to me.