simplify constructors, fixes #564

simplify constructors, fixes #564
we still have an init method, for better or worse, to break out of ES6 restrictions

The idea here is default Dart constructor (really an initialization method) is always called `new` instead of the class name. We already do this for unnamed factory.

R=leafp@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/636fcb68f780bcd38c1aab908dfd432f511df08d

[Jennifer Messerly, 2016-05-11 22:41:57]

Description was changed from

==========
simplify constructors, fixes #564
we still have an init method, for better or worse, to break out of ES6
restrictions
==========

to

==========
simplify constructors, fixes #564
we still have an init method, for better or worse, to break out of ES6
restrictions
==========

[Jennifer Messerly, 2016-05-11 22:41:57]

jmesserly@google.com changed reviewers:
+ leafp@google.com, sra@google.com, vsm@google.com

[Jennifer Messerly, 2016-05-11 22:48:02]

Disclaimers:
* no idea if this improves overall performance, but it removes some of the
bounce logic in core.Object's ES6 constructor
* redirecting constructors are probably slower, but closer to correct. They were
broken if your derived class had a redirecting constructor with the same name
(you'd call the wrong one).
* ES6 classes aren't a great fit for Dart's class init model. Not sure what we
can do in the long term. I haven't come up with any solutions that use
classes+are fast+are correct.

[Jennifer Messerly, 2016-05-11 22:48:48]

Description was changed from

==========
simplify constructors, fixes #564
we still have an init method, for better or worse, to break out of ES6
restrictions
==========

to

==========
simplify constructors, fixes #564
we still have an init method, for better or worse, to break out of ES6
restrictions

The idea here is default Dart constructor (really an initialization method) is
always called `new` instead of the class name. We already do this for unnamed
factory.
==========

[sra1, 2016-05-14 19:36:41]

I think this is a good change.
There was a slight performance improvement in DeltaBlue, but close to noise.

[sra1, 2016-05-14 20:18:27]

V8 still can't optimize DDC constructors.
The issue appears to be use of 'arguments' other than 'arguments.length'.

'⚠ Not optimized: Bad value context for arguments value'

[sra1, 2016-05-15 00:26:33]

On 2016/05/14 20:18:27, sra1 wrote:
> V8 still can't optimize DDC constructors.
> The issue appears to be use of 'arguments' other than 'arguments.length'.
> 
> '⚠ Not optimized: Bad value context for arguments value'

After some fiddling, what I have discovered is that we should generate:

   core.Object = class Object {
     constructor(...args) {
       return this.new.apply(this, args);
     }

This 'mixed metaphor' avoids the 'Not optimized' flag on the core.Object
constructor.

[vsm, 2016-05-16 15:35:40]

On 2016/05/15 00:26:33, sra1 wrote:
> On 2016/05/14 20:18:27, sra1 wrote:
> > V8 still can't optimize DDC constructors.
> > The issue appears to be use of 'arguments' other than 'arguments.length'.
> > 
> > '⚠ Not optimized: Bad value context for arguments value'
> 
> After some fiddling, what I have discovered is that we should generate:
> 
>    core.Object = class Object {
>      constructor(...args) {
>        return this.new.apply(this, args);
>      }
> 
> This 'mixed metaphor' avoids the 'Not optimized' flag on the core.Object
> constructor.

We don't need to get rid of the spread?  I thought that was also unoptimized /
slow?

[sra1, 2016-05-16 15:48:01]

On 2016/05/16 15:35:40, vsm wrote:
> On 2016/05/15 00:26:33, sra1 wrote:
> > On 2016/05/14 20:18:27, sra1 wrote:
> > > V8 still can't optimize DDC constructors.
> > > The issue appears to be use of 'arguments' other than 'arguments.length'.
> > > 
> > > '⚠ Not optimized: Bad value context for arguments value'
> > 
> > After some fiddling, what I have discovered is that we should generate:
> > 
> >    core.Object = class Object {
> >      constructor(...args) {
> >        return this.new.apply(this, args);
> >      }
> > 
> > This 'mixed metaphor' avoids the 'Not optimized' flag on the core.Object
> > constructor.
> 
> We don't need to get rid of the spread?  I thought that was also unoptimized /
> slow?

Spread arguments (call) are slow.
Rest parameters (...parameter) are fast.

[vsm, 2016-05-16 15:52:10]

On 2016/05/16 15:48:01, sra1 wrote:
> On 2016/05/16 15:35:40, vsm wrote:
> > On 2016/05/15 00:26:33, sra1 wrote:
> > > On 2016/05/14 20:18:27, sra1 wrote:
> > > > V8 still can't optimize DDC constructors.
> > > > The issue appears to be use of 'arguments' other than
'arguments.length'.
> > > > 
> > > > '⚠ Not optimized: Bad value context for arguments value'
> > > 
> > > After some fiddling, what I have discovered is that we should generate:
> > > 
> > >    core.Object = class Object {
> > >      constructor(...args) {
> > >        return this.new.apply(this, args);
> > >      }
> > > 
> > > This 'mixed metaphor' avoids the 'Not optimized' flag on the core.Object
> > > constructor.
> > 
> > We don't need to get rid of the spread?  I thought that was also unoptimized
/
> > slow?
> 
> Spread arguments (call) are slow.
> Rest parameters (...parameter) are fast.

Ahh, got it.  Does your fix make a noticeable perf change? 

John is out today - we should land this if we want it.

[Jennifer Messerly, 2016-05-17 17:53:31]

On 2016/05/16 15:52:10, vsm wrote:
> On 2016/05/16 15:48:01, sra1 wrote:
> > On 2016/05/16 15:35:40, vsm wrote:
> > > On 2016/05/15 00:26:33, sra1 wrote:
> > > > On 2016/05/14 20:18:27, sra1 wrote:
> > > > > V8 still can't optimize DDC constructors.
> > > > > The issue appears to be use of 'arguments' other than
> 'arguments.length'.
> > > > > 
> > > > > '⚠ Not optimized: Bad value context for arguments value'
> > > > 
> > > > After some fiddling, what I have discovered is that we should generate:
> > > > 
> > > >    core.Object = class Object {
> > > >      constructor(...args) {
> > > >        return this.new.apply(this, args);
> > > >      }
> > > > 
> > > > This 'mixed metaphor' avoids the 'Not optimized' flag on the core.Object
> > > > constructor.
> > > 
> > > We don't need to get rid of the spread?  I thought that was also
unoptimized
> /
> > > slow?
> > 
> > Spread arguments (call) are slow.
> > Rest parameters (...parameter) are fast.
> 
> Ahh, got it.  Does your fix make a noticeable perf change? 
> 
> John is out today - we should land this if we want it.

Thanks Stephen! That is a great find, I will make that tweak and land it. Yeah I
noticed also that rest params are good. We should check if we are using
"arguments" anywhere else and replace with rest params+.apply, seems like the
fastest V8 pattern for now.

[Jennifer Messerly, 2016-05-17 20:53:41]

On 2016/05/17 17:53:31, John Messerly wrote:
> On 2016/05/16 15:52:10, vsm wrote:
> > On 2016/05/16 15:48:01, sra1 wrote:
> > > On 2016/05/16 15:35:40, vsm wrote:
> > > > On 2016/05/15 00:26:33, sra1 wrote:
> > > > > On 2016/05/14 20:18:27, sra1 wrote:
> > > > > > V8 still can't optimize DDC constructors.
> > > > > > The issue appears to be use of 'arguments' other than
> > 'arguments.length'.
> > > > > > 
> > > > > > '⚠ Not optimized: Bad value context for arguments value'
> > > > > 
> > > > > After some fiddling, what I have discovered is that we should
generate:
> > > > > 
> > > > >    core.Object = class Object {
> > > > >      constructor(...args) {
> > > > >        return this.new.apply(this, args);
> > > > >      }
> > > > > 
> > > > > This 'mixed metaphor' avoids the 'Not optimized' flag on the
core.Object
> > > > > constructor.
> > > > 
> > > > We don't need to get rid of the spread?  I thought that was also
> unoptimized
> > /
> > > > slow?
> > > 
> > > Spread arguments (call) are slow.
> > > Rest parameters (...parameter) are fast.
> > 
> > Ahh, got it.  Does your fix make a noticeable perf change? 
> > 
> > John is out today - we should land this if we want it.
> 
> Thanks Stephen! That is a great find, I will make that tweak and land it. Yeah
I
> noticed also that rest params are good. We should check if we are using
> "arguments" anywhere else and replace with rest params+.apply, seems like the
> fastest V8 pattern for now.

I made Stephen's change and merged it :)

[Jennifer Messerly, 2016-05-17 20:56:13]

On 2016/05/17 20:53:41, John Messerly wrote:
> On 2016/05/17 17:53:31, John Messerly wrote:
> > On 2016/05/16 15:52:10, vsm wrote:
> > > On 2016/05/16 15:48:01, sra1 wrote:
> > > > On 2016/05/16 15:35:40, vsm wrote:
> > > > > On 2016/05/15 00:26:33, sra1 wrote:
> > > > > > On 2016/05/14 20:18:27, sra1 wrote:
> > > > > > > V8 still can't optimize DDC constructors.
> > > > > > > The issue appears to be use of 'arguments' other than
> > > 'arguments.length'.
> > > > > > > 
> > > > > > > '⚠ Not optimized: Bad value context for arguments value'
> > > > > > 
> > > > > > After some fiddling, what I have discovered is that we should
> generate:
> > > > > > 
> > > > > >    core.Object = class Object {
> > > > > >      constructor(...args) {
> > > > > >        return this.new.apply(this, args);
> > > > > >      }
> > > > > > 
> > > > > > This 'mixed metaphor' avoids the 'Not optimized' flag on the
> core.Object
> > > > > > constructor.
> > > > > 
> > > > > We don't need to get rid of the spread?  I thought that was also
> > unoptimized
> > > /
> > > > > slow?
> > > > 
> > > > Spread arguments (call) are slow.
> > > > Rest parameters (...parameter) are fast.
> > > 
> > > Ahh, got it.  Does your fix make a noticeable perf change? 
> > > 
> > > John is out today - we should land this if we want it.
> > 
> > Thanks Stephen! That is a great find, I will make that tweak and land it.
Yeah
> I
> > noticed also that rest params are good. We should check if we are using
> > "arguments" anywhere else and replace with rest params+.apply, seems like
the
> > fastest V8 pattern for now.
> 
> I made Stephen's change and merged it :)

... oops hit send too soon. Let me know if this looks good to land :)

[Leaf, 2016-05-17 21:32:06]

lgtm

[Jennifer Messerly, 2016-05-17 21:37:00]

Description was changed from

==========
simplify constructors, fixes #564
we still have an init method, for better or worse, to break out of ES6
restrictions

The idea here is default Dart constructor (really an initialization method) is
always called `new` instead of the class name. We already do this for unnamed
factory.
==========

to

==========
simplify constructors, fixes #564
we still have an init method, for better or worse, to break out of ES6
restrictions

The idea here is default Dart constructor (really an initialization method) is
always called `new` instead of the class name. We already do this for unnamed
factory.

R=leafp@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/636fcb68f780bcd38c1aab908dfd...
==========

[Jennifer Messerly, 2016-05-17 21:37:02]

Committed patchset #2 (id:20001) manually as
636fcb68f780bcd38c1aab908dfd432f511df08d (presubmit successful).