Encode super calls via extra properties on prototypes.

Encode super calls via extra properties on prototypes.

BUG=
R=floitsch@google.com, sra@google.com

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

[herhut, 2014-11-03 09:26:38]

Patchset #1 (id:1) has been deleted

[herhut, 2014-11-03 09:27:13]

herhut@google.com changed reviewers:
+ karlklose@google.com

[herhut, 2014-11-03 09:28:36]

This changes produces smaller code (not significant when gzipped) but also
speeds up super calls significantly (about a factor of 5 for an artificial
benchmark that does nothing else).

[floitsch, 2014-11-03 09:46:22]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-11-03 09:46:23]

DBC.

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1686:
js.js('#.prototype.#.call(#)', [
indentation.

[sra1, 2014-11-03 18:30:55]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2014-11-03 18:30:56]

LGTM.

I'd like to see the canonical order problem solved before committing since that
is less effort than tracking a bug for the issue.

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_builder.dart
(right):

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_builder.dart:13:
final List<String> aliasedSuperMethods = <String>[];
Maybe allocate on first use since most classes don't have a supercall entry

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_builder.dart:55:
buffer.writeAll(aliasedSuperMethods, ',');
I commented elsewhere about a canoncial order.
Sorting here would work.
Sorting a copy would make writing a non-mutating operation

   buffer.writeAll(aliasedSuperMethods.toList()..sort(), ',');

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart
(right):

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart:323:
object[member] = properties[member];
If you moved the alias logic here (split member), the encoding would be more
compact:

  "foo$3,super$Node$foo$3": function...

Are there problems with doing this?

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1685: push(
What would it take to use an alias for getters too?

[herhut, 2014-11-04 12:40:17]

Patchset #2 (id:40001) has been deleted

[herhut, 2014-11-04 12:45:30]

Patchset #2 (id:60001) has been deleted

[herhut, 2014-11-04 12:52:49]

PTAL

I have changed the implementation to encode the alias names in the property
itself (as sra@ proposed).

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_builder.dart
(right):

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_builder.dart:13:
final List<String> aliasedSuperMethods = <String>[];
On 2014/11/03 18:30:56, sra1 wrote:
> Maybe allocate on first use since most classes don't have a supercall entry

The field is now gone.

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_builder.dart:55:
buffer.writeAll(aliasedSuperMethods, ',');
On 2014/11/03 18:30:56, sra1 wrote:
> I commented elsewhere about a canoncial order.
> Sorting here would work.
> Sorting a copy would make writing a non-mutating operation
> 
>    buffer.writeAll(aliasedSuperMethods.toList()..sort(), ',');

Not required in the new encoding.

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart
(right):

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart:323:
object[member] = properties[member];
On 2014/11/03 18:30:56, sra1 wrote:
> If you moved the alias logic here (split member), the encoding would be more
> compact:
> 
>   "foo$3,super$Node$foo$3": function...
> 
> Are there problems with doing this?

No, great suggestion. Done.

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1685: push(
On 2014/11/03 18:30:56, sra1 wrote:
> What would it take to use an alias for getters too?

Getters are more complicated, as they can be real getters with corresponding
code or generated at runtime. I would have to extend the already complex
encoding for fields with auto generated getters. Given that super getters are
rare (in the usecase I am looking at it is 30 getter calls vs 1450 method
invocations) I have decided to leave them as is.

https://codereview.chromium.org/693883003/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1686:
js.js('#.prototype.#.call(#)', [
On 2014/11/03 09:46:22, floitsch wrote:
> indentation.

Done.

[floitsch, 2014-11-04 13:35:22]

LGTM.

https://codereview.chromium.org/693883003/diff/80001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart
(right):

https://codereview.chromium.org/693883003/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart:383:
* string encoding fields, constructor and superclass. Get the
Gets

[herhut, 2014-11-04 13:55:37]

https://codereview.chromium.org/693883003/diff/80001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart
(right):

https://codereview.chromium.org/693883003/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart:383:
* string encoding fields, constructor and superclass. Get the
On 2014/11/04 13:35:21, floitsch wrote:
> Gets

Done.

[herhut, 2014-11-05 10:25:33]

Patchset #4 (id:120001) has been deleted

[herhut, 2014-11-05 10:35:29]

It turns out that splitting off aliases cannot be piggybacked on inheritFrom, as
the Object object does have a superclass and thus inheritFrom is not applied to
it.

I also had to fix stub generation to differentiate between the propertyname
(which can now hold some additional information) and the actual function name
(currently prefix up to the first : character).

NB: Calling delete on a prototype no longer makes it slow in v8.

[sra1, 2014-11-06 04:24:28]

lgtm

[herhut, 2014-11-06 12:20:03]

Committed patchset #4 (id:140001) manually as 41550 (presubmit successful).

[ahe, 2014-11-10 16:33:09]

ahe@google.com changed reviewers:
+ ahe@google.com

[ahe, 2014-11-10 16:33:10]

This broke https://codereview.chromium.org/701133003.

When you collect new global information in the backend, I'd appreciate a heads
up. By "new" I mean something that wasn't collected before.

In order to support incremental compilation, I need to:

* remove elements from various collections to avoid memory leaks.

* know how to perform an operation incrementally.

I hope this gives a little insight into what I'd like to be made aware of :-)