Closed Bug 636363 Opened 13 years ago Closed 13 years ago

ANI: offer a simple, statically-typed way to create instances

Categories

(Tamarin Graveyard :: Virtual Machine, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stejohns, Assigned: stejohns)

References

Details

Attachments

(4 files)

Existing usage in Flash/AIR has a convoluted, printf/varargs approach to convert C++ values into Atom, call into the class contructor, and return the result. We should instead have nativegen emit wrappers, so C++ code can do something like

    FooObject* foo = toplevel->builtinClasses()->get_FooClass()->construct(1, 3.14, "Happy");
Previously, non-native builtin classes all appeared in the classmanifest as "ClassClosure*", which is accurate but not useful for achieving our goal; this patch creates a "synthetic" wrapper for each non-native builtin class that the next patch will build on. Note that these are truly synthetic, as these C++ classes are never instantiated as such; however, since there's no mechanism for them to live outside the class manifest we should be safe. Also note that they are emitted in a new header file; this is necessary as they must come after ClassClosure in the inclusion process.
Assignee: nobody → stejohns
Attachment #515678 - Flags: review?(rreitmai)
The patch also converts a handful of existing usecases to use the new call; most usage will happen in Flash/AIR glue code.
Attachment #515680 - Flags: review?(rreitmai)
Your generated class should probably also define a private operator= and a private copy constructor unless you can show that those won't work given you current method.  It may be sufficient to show that a private operator= and a private copy construct on the ScriptObject base class will prevent all subclasses from be assigned to or copy constructed.
(In reply to comment #3)
> Your generated class should probably also define a private operator= and a
> private copy constructor unless you can show that those won't work given you
> current method.  It may be sufficient to show that a private operator= and a
> private copy construct on the ScriptObject base class will prevent all
> subclasses from be assigned to or copy constructed.

Good call, I'll add that and see if there are any red flags.
Attachment #515717 - Flags: review?(rreitmai)
Attachment #515678 - Flags: review?(rreitmai) → review+
Attachment #515680 - Flags: review?(rreitmai) → review+
Attachment #515717 - Flags: review?(rreitmai) → review+
changeset: 5995:48b099e59e48
user:      Steven Johnson <stejohns@adobe.com>
summary:   Bug 636363 - ANI: offer a simple, statically-typed way to create instances (r=rreitmai)

http://hg.mozilla.org/tamarin-redux/rev/48b099e59e48
This is awesome!    Will the machinery be reusable for a generic callin mechanism?    Ideally any method could be invoked from C++ like this, although it would probably want to be limited to methods flagged in the actionscript via some metadata.

Should there be an opt-out mechanism, like maybe you don't want folks to construct from C++ for some reason?
(In reply to comment #7)
> This is awesome!    Will the machinery be reusable for a generic callin
> mechanism?    Ideally any method could be invoked from C++ like this, although
> it would probably want to be limited to methods flagged in the actionscript via
> some metadata.

Yep. I haven't decided how/what to expose yet; exposing everything is probably overkill.

> Should there be an opt-out mechanism, like maybe you don't want folks to
> construct from C++ for some reason?

Hmm, good idea, maybe will add in the future.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
re-opening because it occurs to me that supporting optional args would be desirable and simple: just generate multiple overloaded constructObjects, dropping an arg off each time.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #515814 - Flags: review?(rreitmai)
Attachment #515814 - Flags: feedback?(edwsmith)
Passing all the arguments isn't so bad, not sure it's worth increasing the amount of generated code.
(In reply to comment #11)
> Passing all the arguments isn't so bad, not sure it's worth increasing the
> amount of generated code.

Agreed; main motivation is nontrivial amount of existing glue code that takes advantage of the default args; fixing this requires inspecting the AS source and inserting the proper default values. The amount of generated code is pretty much a nonfactor since its inlined and thus theoretically only a price we pay when used.
(In reply to comment #12)
> (In reply to comment #11)
> > Passing all the arguments isn't so bad, not sure it's worth increasing the
> > amount of generated code.
> 
> Agreed; main motivation is nontrivial amount of existing glue code that takes
> advantage of the default args; fixing this requires inspecting the AS source
> and inserting the proper default values. The amount of generated code is pretty
> much a nonfactor since its inlined and thus theoretically only a price we pay
> when used.

The two are semantically different passing a argument that you think is the default value is not the same as letting the vm get the real default value from method info.  What the current patch does is not only *not* a code size issue, it is more correct.
Sounds good then.
Comment on attachment 515814 [details] [diff] [review]
Part 4: support optional-arguments ctors

Seems reasonable but I'm with each passing patch I'm getting more concerned over our use (excessive?) of REALLY_INLINE.  Still tempted to do a speed/size analysis on this aspect when I find some time.
Attachment #515814 - Flags: review?(rreitmai) → review+
(In reply to comment #15)
> Seems reasonable but I'm with each passing patch I'm getting more concerned
> over our use (excessive?) of REALLY_INLINE.  Still tempted to do a speed/size
> analysis on this aspect when I find some time.

Fair point -- the theory I'm operating under on this is that (1) it allows for pay-as-you-go (no need to worry about dodgy linkers missing dead-strip opportunities), (2) "unused" calls we generate have zero cost (aside perhaps some increased compile-time), and (3) the net code size of an inlined constructObject() call of this sort is comparable to that of the old varargs-printf-style constructObject() call.

Of those assumptions, the first two seem pretty sound, but the third probably needs verification.
6027:b37477d7dcf7
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Attachment #515814 - Flags: feedback?(edwsmith)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: