Minifying renamer for classes, methods and instance variables.

Minifying renamer for classes, methods and instance variables.


R=floitsch@google.com
BUG=

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

[floitsch, 2012-10-25 08:42:40]

initial comments.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js/printer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js/printer.dart:988: // since those are the ones we
can generate.
assert this.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/emitter.dart:125: var accessorName =
field = field.substring(0, len - 1);
remove "field =".

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:273: } else if (element.kind
=== ElementKind.LIBRARY) {
element.isLibrary()

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:243: // clashing with the
method names on Object.prototype (needed for native objects
missing closing ")".

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/s...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/s...
lib/compiler/implementation/ssa/codegen.dart:105: String codeString =
"${prettyPrint(codegen.body).toString()}";
why was this necessary?

[erikcorry, 2012-10-25 09:09:28]

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js/printer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js/printer.dart:988: // since those are the ones we
can generate.
On 2012/10/25 08:42:40, floitsch wrote:
> assert this.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/emitter.dart:125: var accessorName =
field = field.substring(0, len - 1);
On 2012/10/25 08:42:40, floitsch wrote:
> remove "field =".

Then it won't work any more!?

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:273: } else if (element.kind
=== ElementKind.LIBRARY) {
On 2012/10/25 08:42:40, floitsch wrote:
> element.isLibrary()

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:243: // clashing with the
method names on Object.prototype (needed for native objects
On 2012/10/25 08:42:40, floitsch wrote:
> missing closing ")".

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/s...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/s...
lib/compiler/implementation/ssa/codegen.dart:105: String codeString =
"${prettyPrint(codegen.body).toString()}";
On 2012/10/25 08:42:40, floitsch wrote:
> why was this necessary?

Whatever the reason was, it went away again.

[floitsch, 2012-10-25 12:58:38]

LGTM.
Please talk to the remaining dart2js about the tests before you submit.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js/printer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js/printer.dart:990: return name == "I";  //
Minified version of Isolate.
Is there a way to get this set from the namer?

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/emitter.dart:111: String
needsGetter(String variable) => '($variable & 3) > 0';
needsGetterCode or something to indicate that this is just the code to do this
check dynamically.
other possibility: needsGetterExpression

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/emitter.dart:125: var accessorName =
field = field.substring(0, len - 1);
On 2012/10/25 09:09:28, erikcorry wrote:
> On 2012/10/25 08:42:40, floitsch wrote:
> > remove "field =".
> 
> Then it won't work any more!?

never mind. wasn't thinking.
Please put in two lines, though.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/emitter.dart:842:
buffer.addCharCode(GETTER_SETTER_CODE + flag);
I would prefer "|" to indicate that this is a bit-operation.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:13: bool get minify =>
true;
I think I prefer "shouldMinify" or "isMinifying", but I agree that there is no
good name.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:41: // Try other
n-character names based on the hash.
Give a small description maybe mentioning that:
- we try 1 to 3 character identifiers.
- we use the hash to create n characters.
- if we don't succeed we try again with a slightly perturbed hash.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:45: var codes =
<int>[_letterNumber(h)];
It probably doesn't matter, but you could allocate the array outside the loop
and just clear it at the beginning of the loop.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:46: var divisor = 26;
62.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:47: usedGlobalNames.add('p'); 
// ISOLATE_PROPERTIES.
The ISOLATE_PROPERTIES for the non-minifying namer is not "p".
Iirc the isolate-properties live on the Isolate-function itself and not in its
prototype. They should not be able to clash with global names.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:69: longName =
stringConstant.value.slowToString();
Are you sure this can happen? Strings are normally inlined at their use-sites.
Still a good idea.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:71: longName = "C";
not for this CL, but this basically means that we run out of short (3-char)
names for constants if there are more than 30, right?

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:83: return
one line?

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:110: String private;
s/private/result

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:119: if (identical(owner, lib)
&&
owner == lib

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/native_helper.dart (left):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:251: if (f != null &&
JS('bool', '!!#.methods', f)) {
While I agree that it probably works, this is "cheating". It's just two
characters...
I would prefer this to be eventually parsed into JS where it then gets rid of
the !!.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:197: // Look up the
inheritance chain, getting the class names and using them
thanks for the comments. makes it much easier to understand.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:248: JS('var', 'void 0');
null

https://codereview.chromium.org/11265020/diff/1/tests/compiler/dart2js/array_...
File tests/compiler/dart2js/array_static_intercept_test.dart (right):

https://codereview.chromium.org/11265020/diff/1/tests/compiler/dart2js/array_...
tests/compiler/dart2js/array_static_intercept_test.dart:17:
Expect.isTrue(generated.contains(r'.add$1(') ||
wouldn't it make sense to copy the test and invoke a minifying compiler instead?

https://codereview.chromium.org/11265020/diff/1/tests/compiler/dart2js/class_...
File tests/compiler/dart2js/class_codegen_test.dart (right):

https://codereview.chromium.org/11265020/diff/1/tests/compiler/dart2js/class_...
tests/compiler/dart2js/class_codegen_test.dart:66: String getClassName(String
generated, int idx) {
index

https://codereview.chromium.org/11265020/diff/1/tests/compiler/dart2js/class_...
tests/compiler/dart2js/class_codegen_test.dart:89: '\\\$\\.$className ?=
?\\{"":\\s*\\[\\],\\s*"super": ?"$superClass"');
Not sure if it's better, but it would get rid of some escapes:
r'\$\.''$className'r'....'

[sra1, 2012-10-26 06:41:45]

https://chromiumcodereview.appspot.com/11265020/diff/1/lib/compiler/implement...
File lib/compiler/implementation/lib/native_helper.dart (right):

https://chromiumcodereview.appspot.com/11265020/diff/1/lib/compiler/implement...
lib/compiler/implementation/lib/native_helper.dart:192: String className =
getTypeNameOf(obj);
Don't call this 'className', it will lead to confusion.
It is not the name of the class.
E.g. the dart:html class '_ButtonElement' has the tag 'HTMLButtonElement'

It is a token or tag that is hopefully unique to the underlying JavsScript  or
DOM object that can be used to look up the method (and hopefully cache the
method on the prototype to avoid repeated lookups).

getTypeNameOf is misnamed.  It should be call getTypeTag.
I does not return the name of any Dart type.

https://chromiumcodereview.appspot.com/11265020/diff/1/lib/compiler/implement...
lib/compiler/implementation/lib/native_helper.dart:197: // Look up the
inheritance chain, getting the class names and using them
It is not really a chain, since it is inverted (parent -> children).  It is
possible to use the current structure for multiple inheritance provided all
classes agree on a order.  This might be useful for implementing mixins without
huge code duplication.

https://chromiumcodereview.appspot.com/11265020/diff/1/lib/compiler/implement...
lib/compiler/implementation/lib/native_helper.dart:269: if (f != null &&
JS('bool', '#.methods', f)) {
I agree the !! is safer.  In the conditional context V8 should eliminate it
completely.  It is possible that checked mode might assert the result is
boolean.

https://chromiumcodereview.appspot.com/11265020/diff/1/lib/compiler/implement...
lib/compiler/implementation/lib/native_helper.dart:283: JS('void', '#[#] = #',
methods, getTypeNameOf(const Object()), dartMethod);
Do we have any reason to believe getTypeNameOf(const Object()) is not 'Object'?

[erikcorry, 2012-12-06 09:38:07]

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js/printer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js/printer.dart:990: return name == "I";  //
Minified version of Isolate.
On 2012/10/25 12:58:38, floitsch wrote:
> Is there a way to get this set from the namer?

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/emitter.dart:111: String
needsGetter(String variable) => '($variable & 3) > 0';
On 2012/10/25 12:58:38, floitsch wrote:
> needsGetterCode or something to indicate that this is just the code to do this
> check dynamically.
> other possibility: needsGetterExpression

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/emitter.dart:842:
buffer.addCharCode(GETTER_SETTER_CODE + flag);
On 2012/10/25 12:58:38, floitsch wrote:
> I would prefer "|" to indicate that this is a bit-operation.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:13: bool get minify =>
true;
On 2012/10/25 12:58:38, floitsch wrote:
> I think I prefer "shouldMinify" or "isMinifying", but I agree that there is no
> good name.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:41: // Try other
n-character names based on the hash.
On 2012/10/25 12:58:38, floitsch wrote:
> Give a small description maybe mentioning that:
> - we try 1 to 3 character identifiers.
> - we use the hash to create n characters.
> - if we don't succeed we try again with a slightly perturbed hash.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:45: var codes =
<int>[_letterNumber(h)];
On 2012/10/25 12:58:38, floitsch wrote:
> It probably doesn't matter, but you could allocate the array outside the loop
> and just clear it at the beginning of the loop.

I tried it and it got uglier, so I'll leave it as it was for now.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/minify_namer.dart:46: var divisor = 26;
On 2012/10/25 12:58:38, floitsch wrote:
> 62.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
File lib/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:47: usedGlobalNames.add('p'); 
// ISOLATE_PROPERTIES.
On 2012/10/25 12:58:38, floitsch wrote:
> The ISOLATE_PROPERTIES for the non-minifying namer is not "p".
> Iirc the isolate-properties live on the Isolate-function itself and not in its
> prototype. They should not be able to clash with global names.

Removed this line.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:69: longName =
stringConstant.value.slowToString();
On 2012/10/25 12:58:38, floitsch wrote:
> Are you sure this can happen? Strings are normally inlined at their use-sites.
> Still a good idea.

This can happen.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:71: longName = "C";
On 2012/10/25 12:58:38, floitsch wrote:
> not for this CL, but this basically means that we run out of short (3-char)
> names for constants if there are more than 30, right?

Yes, after the first 30 non-string constants we start naming them U1, U2, U3,
etc.  If we get better at inlining them this won't matter so much.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:83: return
On 2012/10/25 12:58:38, floitsch wrote:
> one line?

nope.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:110: String private;
On 2012/10/25 12:58:38, floitsch wrote:
> s/private/result

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/j...
lib/compiler/implementation/js_backend/namer.dart:119: if (identical(owner, lib)
&&
On 2012/10/25 12:58:38, floitsch wrote:
> owner == lib

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/native_helper.dart (left):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:251: if (f != null &&
JS('bool', '!!#.methods', f)) {
On 2012/10/25 12:58:38, floitsch wrote:
> While I agree that it probably works, this is "cheating". It's just two
> characters...
> I would prefer this to be eventually parsed into JS where it then gets rid of
> the !!.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:192: String className =
getTypeNameOf(obj);
On 2012/10/26 06:41:45, sra1 wrote:
> Don't call this 'className', it will lead to confusion.
> It is not the name of the class.
> E.g. the dart:html class '_ButtonElement' has the tag 'HTMLButtonElement'
> 
> It is a token or tag that is hopefully unique to the underlying JavsScript  or
> DOM object that can be used to look up the method (and hopefully cache the
> method on the prototype to avoid repeated lookups).
> 
> getTypeNameOf is misnamed.  It should be call getTypeTag.
> I does not return the name of any Dart type.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:197: // Look up the
inheritance chain, getting the class names and using them
On 2012/10/25 12:58:38, floitsch wrote:
> thanks for the comments. makes it much easier to understand.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:197: // Look up the
inheritance chain, getting the class names and using them
On 2012/10/26 06:41:45, sra1 wrote:
> It is not really a chain, since it is inverted (parent -> children).  It is
> possible to use the current structure for multiple inheritance provided all
> classes agree on a order.  This might be useful for implementing mixins
without
> huge code duplication.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:248: JS('var', 'void 0');
On 2012/10/25 12:58:38, floitsch wrote:
> null

Done.  I just used dart null rather than JS null.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:269: if (f != null &&
JS('bool', '#.methods', f)) {
On 2012/10/26 06:41:45, sra1 wrote:
> I agree the !! is safer.  In the conditional context V8 should eliminate it
> completely.  It is possible that checked mode might assert the result is
> boolean.

Done.

https://codereview.chromium.org/11265020/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/native_helper.dart:283: JS('void', '#[#] = #',
methods, getTypeNameOf(const Object()), dartMethod);
On 2012/10/26 06:41:45, sra1 wrote:
> Do we have any reason to believe getTypeNameOf(const Object()) is not
'Object'?

No.

https://codereview.chromium.org/11265020/diff/1/tests/compiler/dart2js/array_...
File tests/compiler/dart2js/array_static_intercept_test.dart (right):

https://codereview.chromium.org/11265020/diff/1/tests/compiler/dart2js/array_...
tests/compiler/dart2js/array_static_intercept_test.dart:17:
Expect.isTrue(generated.contains(r'.add$1(') ||
On 2012/10/25 12:58:38, floitsch wrote:
> wouldn't it make sense to copy the test and invoke a minifying compiler
instead?

Yes.