Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11086022:
Add minifying renamer to the JS printer so locals are called z0, z1, z2, etc. (Closed)
|
Created:
8 years, 2 months ago by erikcorry Modified:
8 years, 2 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd minifying renamer to the JS printer so locals are called z0, z1, z2, etc.
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=13457
Patch Set 1 #
Total comments: 32
Patch Set 2 : Fixed typo #Patch Set 3 : #
Total comments: 3
Messages
Total messages: 6 (0 generated)
LGTM. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... File lib/compiler/implementation/js/printer.dart (right): http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:13: Renamer _renamer; no need to make it private. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:604: var newName = _renamer.getName(ref.name); FYI: I think it can currently happen that the name contains a ".". For example "isolate.foo". This is however correctly handled by your code. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:605: out(newName); out(renamer.getName(ref.name)) ? http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:838: abstract class Renamer { Missing enterScope, leaveScope. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:838: abstract class Renamer { I would call this class 'Namer'. Otherwise it implies that it renames, which it doesn't always do. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:844: class DummyRenamer implements Renamer { Don't like "Dummy". Maybe "PassThroughNamer", "OriginalNamer", "NoMinifyNamer", "VerboseNamer", "SimpleNamer", ... http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:857: _name_number_stack.add(_name_number); _nameNumberStack http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:867: var map = _maps[i]; Personally I would prefer having a type here, but style-guide says it's ok, and I understand that "Map<String, String>" is long. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:868: if (map.containsKey(oldName)) return map[oldName]; String replacement = map[oldName]; if (replacement !== null) return replacement; http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:880: List<Map<String, String>> _maps; We usually have locals on the top of the class. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:880: List<Map<String, String>> _maps; No need to make the fields private. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:880: List<Map<String, String>> _maps; make the maps field final. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:881: List<int> _name_number_stack; make nameNumberStack final.
http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... File lib/compiler/implementation/js/printer.dart (right): http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:844: class DummyRenamer implements Renamer { On 2012/10/09 13:45:43, floitsch wrote: > Don't like "Dummy". Maybe "PassThroughNamer", "OriginalNamer", "NoMinifyNamer", > "VerboseNamer", "SimpleNamer", ... IdentityNamer? http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:853: MinifyRenamer() : _maps = [], _name_number_stack = [], _name_number = 0; _name_number -> _nameNumber _name_number_stack -> _nameNumberStack http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:853: MinifyRenamer() : _maps = [], _name_number_stack = [], _name_number = 0; You could type these lists: _maps = <Map<String, String>[], _nameNumberStack = <int>[]
http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... File lib/compiler/implementation/js/printer.dart (right): http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:844: class DummyRenamer implements Renamer { On 2012/10/09 13:53:42, ngeoffray wrote: > On 2012/10/09 13:45:43, floitsch wrote: > > Don't like "Dummy". Maybe "PassThroughNamer", "OriginalNamer", > "NoMinifyNamer", > > "VerboseNamer", "SimpleNamer", ... > > IdentityNamer? yeah. That's actually the one I was looking for. thanks.
http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... File lib/compiler/implementation/js/printer.dart (right): http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:13: Renamer _renamer; On 2012/10/09 13:45:43, floitsch wrote: > no need to make it private. Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:604: var newName = _renamer.getName(ref.name); On 2012/10/09 13:45:43, floitsch wrote: > FYI: I think it can currently happen that the name contains a ".". For example > "isolate.foo". > This is however correctly handled by your code. ok http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:605: out(newName); On 2012/10/09 13:45:43, floitsch wrote: > out(renamer.getName(ref.name)) ? Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:838: abstract class Renamer { On 2012/10/09 13:45:43, floitsch wrote: > Missing enterScope, leaveScope. Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:838: abstract class Renamer { On 2012/10/09 13:45:43, floitsch wrote: > I would call this class 'Namer'. Otherwise it implies that it renames, which it > doesn't always do. Done and the member variable was also renamed to match. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:844: class DummyRenamer implements Renamer { On 2012/10/09 13:55:35, floitsch wrote: > On 2012/10/09 13:53:42, ngeoffray wrote: > > On 2012/10/09 13:45:43, floitsch wrote: > > > Don't like "Dummy". Maybe "PassThroughNamer", "OriginalNamer", > > "NoMinifyNamer", > > > "VerboseNamer", "SimpleNamer", ... > > > > IdentityNamer? > yeah. That's actually the one I was looking for. thanks. Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:853: MinifyRenamer() : _maps = [], _name_number_stack = [], _name_number = 0; On 2012/10/09 13:53:42, ngeoffray wrote: > _name_number -> _nameNumber > _name_number_stack -> _nameNumberStack Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:853: MinifyRenamer() : _maps = [], _name_number_stack = [], _name_number = 0; On 2012/10/09 13:53:42, ngeoffray wrote: > You could type these lists: > _maps = <Map<String, String>[], _nameNumberStack = <int>[] I'd rather not. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:857: _name_number_stack.add(_name_number); On 2012/10/09 13:45:43, floitsch wrote: > _nameNumberStack Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:867: var map = _maps[i]; On 2012/10/09 13:45:43, floitsch wrote: > Personally I would prefer having a type here, but style-guide says it's ok, and > I understand that "Map<String, String>" is long. I'd rather not. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:868: if (map.containsKey(oldName)) return map[oldName]; On 2012/10/09 13:45:43, floitsch wrote: > String replacement = map[oldName]; > if (replacement !== null) return replacement; Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:880: List<Map<String, String>> _maps; On 2012/10/09 13:45:43, floitsch wrote: > We usually have locals on the top of the class. Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:880: List<Map<String, String>> _maps; On 2012/10/09 13:45:43, floitsch wrote: > No need to make the fields private. Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:880: List<Map<String, String>> _maps; On 2012/10/09 13:45:43, floitsch wrote: > make the maps field final. Done. http://codereview.chromium.org/11086022/diff/1/lib/compiler/implementation/js... lib/compiler/implementation/js/printer.dart:881: List<int> _name_number_stack; On 2012/10/09 13:45:43, floitsch wrote: > make nameNumberStack final. Done.
A general comments.
JSCompiler renames locals mostly to single letter names.
The locals include function parameters, so, for example, almost all* arity-2
functions with a local compile to
function(a,b){var c=...
This regularity is responsible for for much of the code size benefit when
compressed with gzip.
I believe that this code has the above regularity property, but absolute size is
important to gzip performance because copying strings from further away almost
always encodes as more bits.
Can we use single-letter names?
*The exception would be nested functions that actually reference a, b or c from
the enclosing scope.
Rather than renaming during printing, is it possible to structure this kind of
optimization as a correctness-preserving edit of a JavaScript AST or other
structure? It might be simpler to assign 'js' names to the variables and
parameters before printing, visiting each variable once per variable rather than
once per occurrence. Doing a fresh live range analysis might let you use the
same name for parameters, user locals and temps.
Nits: https://codereview.chromium.org/11086022/diff/7001/lib/compiler/implementatio... File lib/compiler/implementation/js/printer.dart (right): https://codereview.chromium.org/11086022/diff/7001/lib/compiler/implementatio... lib/compiler/implementation/js/printer.dart:22: static Namer DetermineRenamer(bool shouldCompressOutput) { This should have been determineRenamer not DetermineRenamer. https://codereview.chromium.org/11086022/diff/7001/lib/compiler/implementatio... lib/compiler/implementation/js/printer.dart:859: MinifyRenamer() : maps = [], nameNumberStack = [], nameNumber = 0; All these initialization expressions can be safely moved to the definition of the fields. https://codereview.chromium.org/11086022/diff/7001/lib/compiler/implementatio... lib/compiler/implementation/js/printer.dart:881: if (maps.length == 0) return oldName; maps.isEmpty() |
