Generative constructor factories for native objects

Generative constructor factories for native objects

The generative constructor factory function 'upgrades' the pre-existing native object.

Future improvements:

1. The lookup of the constructor function could be handled via some kind of reflection.  When relection is finished, we should see if we can 'slice' it to provide the small footprint of the current table and lookup function.

2. Inlining of generative constructor bodies is disabled for native classes - it is broken and needs investigation.

3. dart2js should prevent 'new X.c()' where X.c is a native class generative constructor.

4. dart2js should warn on native class generative constructors that have arguments (the only scenario where arguments work is via superconstructor calls - this might be useful. I have not tested redirections.)

R=kasperl@google.com

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

[ahe, 2013-10-03 07:44:48]

Kasper, could you please take over this review?

https://codereview.chromium.org/25675002/diff/13001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/25675002/diff/13001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:154:
registerStaticUse(member);
This means that all constructors of type literals are enqueued during
resolution. As far as I can tell, this is to support document.register which I
maintain is a really poor fit for Dart, especially dart2js.

I cannot in good conscience approve any changes that cripples dart2js further in
an attempt to support document.register. I think the metadata approach in
polymer.dart is far superior, and I see no reason why we need document.register
to implement polymer.dart.

[sra1, 2013-10-03 17:00:23]

https://codereview.chromium.org/25675002/diff/13001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/25675002/diff/13001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:154:
registerStaticUse(member);
On 2013/10/03 07:44:49, ahe wrote:
> This means that all constructors of type literals are enqueued during
> resolution. As far as I can tell, this is to support document.register which I
> maintain is a really poor fit for Dart, especially dart2js.

I think that isNativeOrExtendsNative can be made to work during resolution to
reduce the set.

> 
> I cannot in good conscience approve any changes that cripples dart2js further
in
> an attempt to support document.register. I think the metadata approach in
> polymer.dart is far superior, and I see no reason why we need
document.register
> to implement polymer.dart.

The metadata approach uses metadata to figure out the arguments to
document.register.
The JavaScript layer document.register is the only API to the browser, we can't
implement anything without that.

When we use Polymer annotations, we will also need to filter the classes that
may be passed to document.register.
I hope we don't have to analyze all the annotations during resolution to figure
out which classes have annotations that will cause some code in some imported
library to ask for ClassMirror.reflectedType on the annotation target
(https://code.google.com/p/dart/issues/detail?id=12607) to pass to
document.register.

[sra1, 2013-10-03 22:48:09]

Hi Kasper,

PTAL.  This is blocking and needs to land before the disruption of the big team
event, preferably by Friday.

Note that there is no magical behaviour unless the class is extending a native
class which is already magical.  This can be restricted to extending specific
classes, but there is not much point since generative constructors on
non-elements can't be used - there is no platform support to call the
constructor unless it is a web component element.

When we start registering via annotations, e.g. for polymer, we will either have
to generate the constructors for all subclasses of Element, or from some list
provided via an annotation.

[kasperl, 2013-10-04 11:08:42]

LGTM. So with this latest patch set we're only enqueuing generative constructors
of native (or indirectly native) classes that are used as type literals, right?
Not the constructors of all classes used as type literals.

If possible it would be nice to avoid introducing overhead when
document.register isn't used. It probably requires just collecting all the
native classes used as type literals in a set somewhere and do the registering
of the constructors late. Maybe some of the functionality could be put in the
native enqueuer?

https://codereview.chromium.org/25675002/diff/13001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/25675002/diff/13001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1457: // The 'inputs'
are [reciever, arg1, ..., argN] or
receiver

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:965:
classElement.ensureResolved(compiler);
You may want to consider if you can avoid the static use registering if
document.register isn't being used.

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:966: if
(Elements.isNativeOrExtendsNative(classElement)) {
I guess we are sure that we're only calling Elements.isNativeOrExtendsNative on
resolved class elements. I guess it would be nice if we checked that in the
implementation Elements.isNativeOrExtendsNative because the super class
reference will not be correct if the class hasn't been resolved yet.

Maybe we already have that check indirectly through the ClassElement.superclass
accessor but it might make sense to add it to Elements.isNativeOrExtendsNative
to better document the intent.

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:968:
(ClassElement enclosingClass, Element member) {
Maybe this would be more readable if you introduced a local helper function with
a name for it?

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1508:
// Create JavaScript Object map for by-name lookup of generative
Can we avoid this if document.register isn't being used? Maybe add an example
map to the comment that shows what it maps from and to?

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1783: if
(!isNativeUpgradeFactory && // TODO(sra): Fix inlining.
Isn't this check handled by tryInlineMethod?

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1824: if
(constant.isType()) {
This is also only necessary if document.register is used, right?

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/25675002/diff/24001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1457: // The 'inputs'
are [reciever, arg1, ..., argN] or
receiver (twice)

[sra1, 2013-10-04 20:21:08]

Committed patchset #3 manually as r28278 (presubmit successful).

[sra1, 2013-10-04 20:21:28]

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:965:
classElement.ensureResolved(compiler);
On 2013/10/04 11:08:43, kasperl wrote:
> You may want to consider if you can avoid the static use registering if
> document.register isn't being used.

I'll TODO it for now.

It would be nice if there was a single general mechanism for this complex kind
of rule that allowed us to write the rule in one place rather than implement it
as a nest of notifications and state smeared across the enqueuer and backend.

The enqueuer is basically an ad-hoc implementation of the RETE algorithm, which
has a lot in common with semi-naive bottom-up evaluation of datalog programs.

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:966: if
(Elements.isNativeOrExtendsNative(classElement)) {
On 2013/10/04 11:08:43, kasperl wrote:
> I guess we are sure that we're only calling Elements.isNativeOrExtendsNative
on
> resolved class elements. I guess it would be nice if we checked that in the
> implementation Elements.isNativeOrExtendsNative because the super class
> reference will not be correct if the class hasn't been resolved yet.
> 
> Maybe we already have that check indirectly through the
ClassElement.superclass
> accessor but it might make sense to add it to Elements.isNativeOrExtendsNative
> to better document the intent.

Added assertion in isNativeOrExtendsNative.

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:968:
(ClassElement enclosingClass, Element member) {
On 2013/10/04 11:08:43, kasperl wrote:
> Maybe this would be more readable if you introduced a local helper function
with
> a name for it?

Done.

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1508:
// Create JavaScript Object map for by-name lookup of generative
On 2013/10/04 11:08:43, kasperl wrote:
> Can we avoid this if document.register isn't being used?

It would be slightly more complicated than that - it is actually the helpers
that document.register calls that should be the trigger.

 Maybe add an example
> map to the comment that shows what it maps from and to?

Done.

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1783: if
(!isNativeUpgradeFactory && // TODO(sra): Fix inlining.
On 2013/10/04 11:08:43, kasperl wrote:
> Isn't this check handled by tryInlineMethod?

No, that is for generative constructor factories being inlined at 'new' sites.

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1824: if
(constant.isType()) {
On 2013/10/04 11:08:43, kasperl wrote:
> This is also only necessary if document.register is used, right?

Yes, but this is the wrong place for a check.

The filtering will happen in registerEscapingConstructorsOfClass, which has a
TODO to defer the registering until the trigger is seen.

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/24001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1457: // The 'inputs'
are [reciever, arg1, ..., argN] or
On 2013/10/04 11:08:43, kasperl wrote:
> receiver (twice)

Done.

[ngeoffray, 2013-10-17 09:08:59]

LGTM

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:965: // Web
component classes have constructors that are escaped to the host
Please define 'escape' here.

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:278: bool
isNativeUpgradeFactory = element.isGenerativeConstructor()
What is a native upgrade factory?

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:303:
value.instructionType = builder.getTypeOfThis();
This seems very similar to the code line 281 to 295. What prevents the sharing?
Can't you just change line 291 to also check isNativeUpgradeFactory?

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1610: HInstruction
value = graph.addConstantNull(compiler);
Avoid the temporary?

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1624:
fieldValues[member] = value;
Avoid the temporary?

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1799: js.Expression
constructor = new js.VariableUse(jsClassReference);
Why this change?

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1825: // If the type
is a web component, we need to ensure the constructors are
There's no check of 'web component' here. Maybe you should check it here and
assert in 'regiterEscapingConstructorsOfClass'.

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/lib/int...
File sdk/lib/_internal/lib/interceptors.dart (right):

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/lib/int...
sdk/lib/_internal/lib/interceptors.dart:166: int
findIndexForWebComponentType(Type type) {
I find the term web component too high level, and subject to change. Can't you
just use Native or SubNative?

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/lib/int...
sdk/lib/_internal/lib/interceptors.dart:170: for (int i = 0; i + 1 < map.length;
i += 3) {
Please add a variable name for the 3.

https://codereview.chromium.org/25675002/diff/37001/sdk/lib/_internal/lib/int...
sdk/lib/_internal/lib/interceptors.dart:191:
findConstructorForWebComponentType(Type type, String name) {
Same comment for web component.

[sra1, 2013-10-18 04:09:31]

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:965: // Web
component classes have constructors that are escaped to the host
On 2013/10/17 09:08:59, ngeoffray wrote:
> Please define 'escape' here.

This code has been replaced.

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:278: bool
isNativeUpgradeFactory = element.isGenerativeConstructor()
On 2013/10/17 09:08:59, ngeoffray wrote:
> What is a native upgrade factory?

Added comment

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:303:
value.instructionType = builder.getTypeOfThis();
On 2013/10/17 09:08:59, ngeoffray wrote:
> This seems very similar to the code line 281 to 295. What prevents the
sharing?
> Can't you just change line 291 to also check isNativeUpgradeFactory?

In the fixed version there are more differences, enough to make separate
branches no longer the the version you propose.

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1610: HInstruction
value = graph.addConstantNull(compiler);
On 2013/10/17 09:08:59, ngeoffray wrote:
> Avoid the temporary?

Done.

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1624:
fieldValues[member] = value;
On 2013/10/17 09:08:59, ngeoffray wrote:
> Avoid the temporary?

Done.

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1799: js.Expression
constructor = new js.VariableUse(jsClassReference);
On 2013/10/17 09:08:59, ngeoffray wrote:
> Why this change?

It can be changed back.  Original formulation called the constructor instead of
new-ing it, but that had the bad effect of null-ing out native fields.

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1825: // If the type
is a web component, we need to ensure the constructors are
On 2013/10/17 09:08:59, ngeoffray wrote:
> There's no check of 'web component' here. Maybe you should check it here and
> assert in 'regiterEscapingConstructorsOfClass'.

We now register the constant with the backend.

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
File sdk/lib/_internal/lib/interceptors.dart (right):

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/lib/interceptors.dart:166: int
findIndexForWebComponentType(Type type) {
On 2013/10/17 09:08:59, ngeoffray wrote:
> I find the term web component too high level, and subject to change. Can't you
> just use Native or SubNative?

Done.

https://chromiumcodereview.appspot.com/25675002/diff/37001/sdk/lib/_internal/...
sdk/lib/_internal/lib/interceptors.dart:191:
findConstructorForWebComponentType(Type type, String name) {
On 2013/10/17 09:08:59, ngeoffray wrote:
> Same comment for web component.

Done.