634635 - ANI: nativegen should emit synthetic C++ classes for all builtin classes, native or not

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

For pure-AS3 classes in the builtins (eg "Point"), nativegen should emit C++ wrappers; this would greatly simplify various builtin bits of code that currently uses helper classes (eg Slot<>) to access these classes efficiently.

Comment 1

[Jeff Mott]

Yes!

Comment 2

[Edwin Smith]

Should this be opt-in?  generating C++ visible entry points for all AS3 builtin code could be overkill.

Comment 3

[Steven Johnson]

Maybe opt-in, not sure yet -- gonna experiment and see first.

Comment 4

[Steven Johnson]

I've done some work on this and it looks doable, but I'm wondering if a path of less resistance is just to have the classes we want accessible this way be declared as "native" for this purpose... I'm gonna look at existing use cases, if that will suffice I'll retire this.

Comment 5

[Steven Johnson]

After doing some prototyping, my concern with generating "synthetic" classes is that they really will be synthetic: ie, we can easily generate a wrapper for (say) "Point" that will expose getters and setters for AS3 data fields, but the C++ "PointObject" class will never actually be allocated as such; we'll really be allocating a C++ ScriptObject with extra memory on the end, the appropriate vtable... the "PointObject" would be there only for syntactic sugar. I'm concerned that having this sort of "kinda sorta" C++ class in existence will only serve to confuse glue coders, who are confused enough (for good reason) as it is.

Looking over existing usage, it appears that there are only a handful of use cases for this in existing glue code (less than a dozen); I suspect a simpler approach would be to make those classes "native" just to expose the slots that way, which would allow us to eliminate Slot<> entirely (which is the main goal of this bug).

That said, I'd welcome feedback from Chris and Jeff on this.

Comment 6

[Jeff Mott]

Eliminating Slot is good.  Simple is good.  Just taking those classes native seems perfectly reasonable, and simpler than auto-magically generating C++ classes.

Comment 7

[Christopher A. Brichford]

(In reply to comment #5)
> After doing some prototyping, my concern with generating "synthetic" classes is
> that they really will be synthetic: ie, we can easily generate a wrapper for
> (say) "Point" that will expose getters and setters for AS3 data fields, but the
> C++ "PointObject" class will never actually be allocated as such; we'll really
> be allocating a C++ ScriptObject with extra memory on the end, the appropriate
> vtable... the "PointObject" would be there only for syntactic sugar. I'm
> concerned that having this sort of "kinda sorta" C++ class in existence will
> only serve to confuse glue coders, who are confused enough (for good reason) as
> it is.
> 
If the only way to get a pointer to a PointObject is some sort of static methods that accepts a ScriptObject*, I don't think it is that weird.   The generated class would have private constructors to prevent it from being constructed.  What you are proposing to do is to just generate the same C++ class by hand.  You'll have to have new glue headers and source files for those classes.  Those in themselves will be weird in that they won't contain any native methods.

For what is worth, you don't have to generate C++ classes.  You could just generated functions of the form:
boolean get_flash_utils_MyRandomClass_private_myBoolean(ScriptObject* obj);
for something like:

package flash.utils
{
public class MyRandomClass
{
  private var myBoolean : Boolean;
}
}


> Looking over existing usage, it appears that there are only a handful of use
> cases for this in existing glue code (less than a dozen); I suspect a simpler
> approach would be to make those classes "native" just to expose the slots that
> way, which would allow us to eliminate Slot<> entirely (which is the main goal
> of this bug).
> 

We don't want to just get rid of the slot template, we want to get rid of any code that accesses a slot through any mechanism other than a getter or setter generated by nativegen.py.

If you generate C++ classes for everything maybe things like Rectangle.as and Point.as would not need C++ glue classes.

Comment 8

[Steven Johnson]

(In reply to comment #5)
> After doing some prototyping, my concern with generating "synthetic" classes is
> that they really will be synthetic: ie, we can easily generate a wrapper for
> (say) "Point" that will expose getters and setters for AS3 data fields, but the
> C++ "PointObject" class will never actually be allocated as such; we'll really
> be allocating a C++ ScriptObject with extra memory on the end, the appropriate
> vtable... the "PointObject" would be there only for syntactic sugar.

Revisiting this once again, it now occurs to me that with other changes that have landed (eg in the construction pipeline) we could probably do this the "right" way and generate 100% real C++ classes for these, which would simplify some other things (eg callins) considerably. I'm going to revisit it with that in mind.

Comment 9

[Steven Johnson]

Attachment: Patch

OK, I really like this patch: for AS3 classes that are processed by nativegen, and do NOT have native metadata, we emit C++ class/object files that conform to expected form.

Why is this cool?
-- Allows you to use slot accessors for all builtin classes, whether it's really "native" or not. Thus Slot<> and friends will go away.
-- Ditto for soon-to-arrive callin wrappers.
-- There's an strongly-typed C++ equivalent to every AS3 type, so you no longer have to downcast to the nearest "native" type in C++ code; previously an all AS3 type was usually just "ScriptObject" in native code, now we can leverage the C++ type system.

Also: I nuked the "constsetters" option and just always emit setters, but with a "setconst_" prefix. The only code anywhere that actually needs 'em is Sampler, so hopefully we can remove them entirely someday.

Comment 10

[Christopher A. Brichford]

Comment on attachment 519820 [details] [diff] [review]
Patch

R+.  One thing I'm not sure about is if you are adding non-native classes to the array of AVMTHUNK_NATIVE_CLASS array.  That is probably ok, but it will make the array a tiny bit bigger and may have some small performance impact depending on how much the path for constructing a "native" class differs from constructing a "non-native" class.  It would be worth clarifying the situation.

Comment 11

[Steven Johnson]

(In reply to comment #10)
> R+.  One thing I'm not sure about is if you are adding non-native classes to
> the array of AVMTHUNK_NATIVE_CLASS array.  That is probably ok, but it will
> make the array a tiny bit bigger and may have some small performance impact
> depending on how much the path for constructing a "native" class differs from
> constructing a "non-native" class.  It would be worth clarifying the situation.

Yep, I toyed with doing it both ways -- we *could* have non-native classes just be a facade on top of a ScriptObject-with-extra-memory, and it would probably be harmless, as the objects should be bitwise-identical. In terms of perf, it should be a wash, as the path is essentially the same, just calling a different ctor, but the different ctor is an empty inlined one.

Comment 12

[Steven Johnson]

Attachment: Patch v2

Updated patch with a few minor fixes, found during work in Flash.

Comment 13

[Jeff Mott]

Still can't figure out how to manage my feedback status, but I like where this is going.

Comment 14

[Rick Reitmaier]

Comment on attachment 520087 [details] [diff] [review]
Patch v2

nativegen is probably now at a level of sophistication that we'll want to have a more formal specification for it; assuming I haven't overlooked it.

Comment 15

[Steven Johnson]

Note followup Bug 645409 (nativegen should emit interface wrappers too)

Comment 16

[Steven Johnson]

TR 6130:a7da387936d6

Comment 17

[Tamarin Bot]

changeset: 6135:86d806fed151
user:      Steven Johnson <[e-mail]>
summary:   Bug 634635 followup: some changes to nativegen.py didn't get pushed (wrong patch file), this is the rest of the changes (r=same)

http://hg.mozilla.org/tamarin-redux/rev/86d806fed151