dart2js: Create noSuchMethod handlers at runtime to reduce overhead.

dart2js: Create noSuchMethod handlers at runtime to reduce overhead.

This reduces the overhead for using noSuchMethod from over 10% to around 2%.
Following this change the original source names are not present when
minifying.  This means you get the minified name in the methodName property
on InvocationMirror objects.

R=ngeoffray@google.com
BUG=

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

[ngeoffray, 2013-03-11 09:41:27]

Initial comments.

https://codereview.chromium.org/12499005/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/12499005/diff/1/pkg/pkg.status#newcode86
pkg/pkg.status:86: # The unittest package relies on getting the original
(non-minified) method
I don't know how specified is the name of InvocationMirror, but if we wanted to
support the real name today, it's not so difficult: just emit a map of renames,
right?

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:523: String
whatToPatch = nativeEmitter.objectToPutNoSuchMethodHandlersOn;
Instead of having these [objectToPush..] getters, I would put the logic in here:
If nativeEmitter has the handleNoSuchMethod flag, then we need to use
Object.prototype, otherwise...

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:546: return
"objectClassObject";
What is this?

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:635: // The
minifier together with noSuchMethod can methods on the
can methods -> can put methods

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2200: bool
trivialNsmHandler(
boolify the name? eg isTrivialNsmHandler?

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2209:
MAX_MINIFIED_LENGTH_FOR_DIFF_ENCODING) return false;
Strange indentation. I would create a local variable for
namer.invocationMirrorInternalName(selector).

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2265:
js.string(compiler.enableMinification ?
Is that to keep the behavior consistent? Ie, always fail in minify mode?

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2360: bool
veryFewNoSuchMemberHandlers = (addedJsNames.length < 10);
Could that be done earlier, to avoid having to pass it as parameter.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2360: bool
veryFewNoSuchMemberHandlers = (addedJsNames.length < 10);
Please move this 10 into a field in this class and put a comment on it.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart
(right):

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart:89: //
cat out.js |
out.js of what?

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart:105:
r'$tdiv']);
Does this list still apply to the command above?

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:40: final
List<Selector> trivialNsmHandlers;
It does not look right to put something here, that is specific to the emitter.

[erikcorry, 2013-03-13 08:55:45]

https://codereview.chromium.org/12499005/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/12499005/diff/1/pkg/pkg.status#newcode86
pkg/pkg.status:86: # The unittest package relies on getting the original
(non-minified) method
On 2013/03/11 09:41:27, ngeoffray wrote:
> I don't know how specified is the name of InvocationMirror, but if we wanted
to
> support the real name today, it's not so difficult: just emit a map of
renames,
> right?

That's right, that's what we do in the non-minifying case.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:523: String
whatToPatch = nativeEmitter.objectToPutNoSuchMethodHandlersOn;
On 2013/03/11 09:41:27, ngeoffray wrote:
> Instead of having these [objectToPush..] getters, I would put the logic in
here:
> If nativeEmitter has the handleNoSuchMethod flag, then we need to use
> Object.prototype, otherwise...

Done.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:546: return
"objectClassObject";
On 2013/03/11 09:41:27, ngeoffray wrote:
> What is this?

Gone.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:635: // The
minifier together with noSuchMethod can methods on the
On 2013/03/11 09:41:27, ngeoffray wrote:
> can methods -> can put methods

Done.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2200: bool
trivialNsmHandler(
On 2013/03/11 09:41:27, ngeoffray wrote:
> boolify the name? eg isTrivialNsmHandler?

Done.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2200: bool
trivialNsmHandler(
On 2013/03/11 09:41:27, ngeoffray wrote:
> boolify the name? eg isTrivialNsmHandler?

Done.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2209:
MAX_MINIFIED_LENGTH_FOR_DIFF_ENCODING) return false;
On 2013/03/11 09:41:27, ngeoffray wrote:
> Strange indentation. I would create a local variable for
> namer.invocationMirrorInternalName(selector).

Done.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2265:
js.string(compiler.enableMinification ?
On 2013/03/11 09:41:27, ngeoffray wrote:
> Is that to keep the behavior consistent? Ie, always fail in minify mode?

Yes.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2360: bool
veryFewNoSuchMemberHandlers = (addedJsNames.length < 10);
On 2013/03/11 09:41:27, ngeoffray wrote:
> Could that be done earlier, to avoid having to pass it as parameter.

I don't understand this question.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2360: bool
veryFewNoSuchMemberHandlers = (addedJsNames.length < 10);
On 2013/03/11 09:41:27, ngeoffray wrote:
> Please move this 10 into a field in this class and put a comment on it.

Done.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart
(right):

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart:89: //
cat out.js |
On 2013/03/11 09:41:27, ngeoffray wrote:
> out.js of what?

I think I'll remove the comment.  It's mainly hand selected at this point.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart:105:
r'$tdiv']);
On 2013/03/11 09:41:27, ngeoffray wrote:
> Does this list still apply to the command above?

Not really.

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12499005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:40: final
List<Selector> trivialNsmHandlers;
On 2013/03/11 09:41:27, ngeoffray wrote:
> It does not look right to put something here, that is specific to the emitter.

Moved to the emitter.

[ngeoffray, 2013-03-13 15:03:27]

LGTM!

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:338: // If we
need fewer than this many noSuchMethod handlers we can save space by
indentation off by one

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:429: // Big
endian encoding, A = 0, B = 1, etc. terminate with lower case.
terminate -> terminates

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:442: bool
minify = compiler.enableMinification;
Isn't that always true in this method?

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:531:
"objectClassObject";
What is objectClassObject?

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2206: // Check
for wrong calling convention.
wrong :-)? maybe change it to "interceptor"?

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2208: // We
can handle the strange calling convention used by intercepted names
strange -> interceptor

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2210: if
(!compiler.enableMinification) return false;
Isn't this code only used by the minifier?

[erikcorry, 2013-03-14 10:06:57]

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:338: // If we
need fewer than this many noSuchMethod handlers we can save space by
On 2013/03/13 15:03:27, ngeoffray wrote:
> indentation off by one

Fixed

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:429: // Big
endian encoding, A = 0, B = 1, etc. terminate with lower case.
On 2013/03/13 15:03:27, ngeoffray wrote:
> terminate -> terminates

This was an imperative verb, not a singular one.

Reformulated to be clearer.

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:442: bool
minify = compiler.enableMinification;
On 2013/03/13 15:03:27, ngeoffray wrote:
> Isn't that always true in this method?

No, but we only diff encode if we are minifying.  In the non-minifying case we
just use a list of mangled names and a parallel list of source names to generate
the noSuchMethod handlers.

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:531:
"objectClassObject";
On 2013/03/13 15:03:27, ngeoffray wrote:
> What is objectClassObject?

It's the JS object corresponding to the Dart Object class.

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2206: // Check
for wrong calling convention.
On 2013/03/13 15:03:27, ngeoffray wrote:
> wrong :-)? maybe change it to "interceptor"?

Done.

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2208: // We
can handle the strange calling convention used by intercepted names
On 2013/03/13 15:03:27, ngeoffray wrote:
> strange -> interceptor

Removed word 'strange'.

https://codereview.chromium.org/12499005/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2210: if
(!compiler.enableMinification) return false;
On 2013/03/13 15:03:27, ngeoffray wrote:
> Isn't this code only used by the minifier?

no

[erikcorry, 2013-03-14 11:49:27]

Committed patchset #3 manually as r20005 (presubmit successful).