dart2js: Implement frequency based naming.

dart2js: Implement frequency based naming.
BUG=
R=sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/9fa33e4e72d4d3d84598863e189d465c26e58841

[herhut, 2015-06-30 16:29:46]

herhut@google.com changed reviewers:
+ floitsch@google.com, sra@google.com

[herhut, 2015-06-30 16:33:26]

Here is a first go at implementing frequency based naming. It also fixes the
size regression in names for one shot interceptors.

This can be further optimized by for instance taking holders into account or
optionally using 2-byte UTF8 characters for names. 

I have marked this as experimental as it currently is not tested by the bots
(and there is no support in test.py at all, yet).

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/field_naming_mixin.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/field_naming_mixin.dart:74: assert(index ==
nameStore.length);
this is a back-ported fix from a later revision...

[sra1, 2015-06-30 16:59:31]

js_lib lgtm.
I'll look at the rest later today.

[sra1, 2015-07-01 04:11:10]

Please find a way to add some tests for some of the machinery.

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/apiimp...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/apiimp...
pkg/compiler/lib/src/apiimpl.dart:82: useFrequencyNamer: hasOption(options,
"--use-frequency-naming"),
Maybe move this next to --minify since that is what it modifies.
Pick a name relates it to minified code, e.g.  --minify-by-frequency

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/dart2j...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/dart2j...
pkg/compiler/lib/src/dart2js.dart:410: if (checkedMode && trustTypeAnnotations)
{
Maybe put a check like this that the experimental option is valid only with
--minify

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js/js....
File pkg/compiler/lib/src/js/js.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js/js....
pkg/compiler/lib/src/js/js.dart:128: /// node.
Perhaps say here why you might want such a strange node?

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/frequency_namer.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/frequency_namer.dart:95: bool
_incrementPosition(int pos) {
Add a comment to say what sequence this generates.
Add a unit test for this class.

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/namer_names.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/namer_names.dart:150: TokenScope _scope;
Can _scope, _proposed be final?

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/metadata_collector.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:235: String
nameToHashCode(jsAst.Name name) => "${name.hashCode}";
Q1. Are you guaranteeing somewhere that each unique name in the program has only
one instance of a jsAst.Name? That would make sense for refcounting.

Q2. What if the hashCodes are not unique?
At least document a TODO to investigate.
Maybe _NamerName should implement a hashString() method that returns a
structurally unique string for this purpose.

[herhut, 2015-07-02 08:54:51]

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/apiimp...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/apiimp...
pkg/compiler/lib/src/apiimpl.dart:82: useFrequencyNamer: hasOption(options,
"--use-frequency-naming"),
On 2015/07/01 04:11:09, sra1 wrote:
> Maybe move this next to --minify since that is what it modifies.
> Pick a name relates it to minified code, e.g.  --minify-by-frequency

After talking to floitsch@, I have now enabled frequency based naming by default
and added --no-frequency-based-minification to enable the old behavior.

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/dart2j...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/dart2j...
pkg/compiler/lib/src/dart2js.dart:410: if (checkedMode && trustTypeAnnotations)
{
On 2015/07/01 04:11:09, sra1 wrote:
> Maybe put a check like this that the experimental option is valid only with
> --minify

As I now use --no-frequency-based-minification, I would like to allow the one
without the other, so that I can configure minification in an alias and still
turn it on or off with a single flag.

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js/js....
File pkg/compiler/lib/src/js/js.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js/js....
pkg/compiler/lib/src/js/js.dart:128: /// node.
On 2015/07/01 04:11:09, sra1 wrote:
> Perhaps say here why you might want such a strange node?

Done.

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/frequency_namer.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/frequency_namer.dart:95: bool
_incrementPosition(int pos) {
On 2015/07/01 04:11:09, sra1 wrote:
> Add a comment to say what sequence this generates.
> Add a unit test for this class.

Done.

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/namer_names.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/namer_names.dart:150: TokenScope _scope;
On 2015/07/01 04:11:09, sra1 wrote:
> Can _scope, _proposed be final?

Yes they can! Thanks.

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/metadata_collector.dart (right):

https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:235: String
nameToHashCode(jsAst.Name name) => "${name.hashCode}";
On 2015/07/01 04:11:09, sra1 wrote:
> Q1. Are you guaranteeing somewhere that each unique name in the program has
only
> one instance of a jsAst.Name? That would make sense for refcounting.

Yes. freshName (which hands out jsAst.Name) produces a fresh, unique name. The
Namer makes sure that consecutive calls with the same element or entity that
needs naming will result in the same name.

However, if the same name is constructed in different ways (like a literal
string gx and a getter with x) this does not hold. That is documented in the
namer and if that is violated, reference counting is the smallest issue.

> Q2. What if the hashCodes are not unique?
> At least document a TODO to investigate.
> Maybe _NamerName should implement a hashString() method that returns a
> structurally unique string for this purpose.

True. I will add a TODO and implement unique keys in a follow-up.

[herhut, 2015-07-02 11:36:21]

On 2015/07/02 08:54:51, herhut wrote:
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/apiimp...
> File pkg/compiler/lib/src/apiimpl.dart (right):
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/apiimp...
> pkg/compiler/lib/src/apiimpl.dart:82: useFrequencyNamer: hasOption(options,
> "--use-frequency-naming"),
> On 2015/07/01 04:11:09, sra1 wrote:
> > Maybe move this next to --minify since that is what it modifies.
> > Pick a name relates it to minified code, e.g.  --minify-by-frequency
> 
> After talking to floitsch@, I have now enabled frequency based naming by
default
> and added --no-frequency-based-minification to enable the old behavior.
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/dart2j...
> File pkg/compiler/lib/src/dart2js.dart (right):
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/dart2j...
> pkg/compiler/lib/src/dart2js.dart:410: if (checkedMode &&
trustTypeAnnotations)
> {
> On 2015/07/01 04:11:09, sra1 wrote:
> > Maybe put a check like this that the experimental option is valid only with
> > --minify
> 
> As I now use --no-frequency-based-minification, I would like to allow the one
> without the other, so that I can configure minification in an alias and still
> turn it on or off with a single flag.
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js/js....
> File pkg/compiler/lib/src/js/js.dart (right):
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js/js....
> pkg/compiler/lib/src/js/js.dart:128: /// node.
> On 2015/07/01 04:11:09, sra1 wrote:
> > Perhaps say here why you might want such a strange node?
> 
> Done.
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
> File pkg/compiler/lib/src/js_backend/frequency_namer.dart (right):
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
> pkg/compiler/lib/src/js_backend/frequency_namer.dart:95: bool
> _incrementPosition(int pos) {
> On 2015/07/01 04:11:09, sra1 wrote:
> > Add a comment to say what sequence this generates.
> > Add a unit test for this class.
> 
> Done.
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
> File pkg/compiler/lib/src/js_backend/namer_names.dart (right):
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_bac...
> pkg/compiler/lib/src/js_backend/namer_names.dart:150: TokenScope _scope;
> On 2015/07/01 04:11:09, sra1 wrote:
> > Can _scope, _proposed be final?
> 
> Yes they can! Thanks.
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_emi...
> File pkg/compiler/lib/src/js_emitter/metadata_collector.dart (right):
> 
>
https://codereview.chromium.org/1212613009/diff/1/pkg/compiler/lib/src/js_emi...
> pkg/compiler/lib/src/js_emitter/metadata_collector.dart:235: String
> nameToHashCode(jsAst.Name name) => "${name.hashCode}";
> On 2015/07/01 04:11:09, sra1 wrote:
> > Q1. Are you guaranteeing somewhere that each unique name in the program has
> only
> > one instance of a jsAst.Name? That would make sense for refcounting.
> 
> Yes. freshName (which hands out jsAst.Name) produces a fresh, unique name. The
> Namer makes sure that consecutive calls with the same element or entity that
> needs naming will result in the same name.
> 
> However, if the same name is constructed in different ways (like a literal
> string gx and a getter with x) this does not hold. That is documented in the
> namer and if that is violated, reference counting is the smallest issue.
> 
> > Q2. What if the hashCodes are not unique?
> > At least document a TODO to investigate.
> > Maybe _NamerName should implement a hashString() method that returns a
> > structurally unique string for this purpose.
> 
> True. I will add a TODO and implement unique keys in a follow-up.

I have instead implemented hash key support right away. Name now has a String
getter key.

[herhut, 2015-07-02 14:47:40]

sra, PTAL

I wonder how we ensure that local variables do not clash with holders? Because
if we have enough of them, they should eventually shadow the holders, no?

[herhut, 2015-07-03 08:41:25]

Patchset #6 (id:90001) has been deleted

[herhut, 2015-07-03 08:42:14]

On 2015/07/02 14:47:40, herhut wrote:
> sra, PTAL
> 
> I wonder how we ensure that local variables do not clash with holders? Because
> if we have enough of them, they should eventually shadow the holders, no?

I still wonder how we ensure this but I fixed the collision with globals.
Globals, as in the ones coming from the namer, have always to reside inside a
holder, which are the objects A-Z and $ for the current isolate.

[herhut, 2015-07-03 11:06:40]

Committed patchset #5 (id:80001) manually as
9fa33e4e72d4d3d84598863e189d465c26e58841 (presubmit successful).