638956 - ANI: Add a clean way to do isType/asType/coerceToType

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

It's common to do these operations in outside-the-VM glue code, but doing so requires access to the Traits type, which we'd like to reduce knowledge of.

Comment 1

[Steven Johnson]

Attachment: Patch

This adds wrapper methods to all of the class stubs. Since all builtin classes are available via the classmanifest, this allows you to do

    builtinClasses()->get_StringClass()->isType(someAtom);

    GCRef<ArrayObject> builtinClasses()->get_ArrayClass()->asType(someAtom);

    GCRef<ArrayObject> builtinClasses()->get_ArrayClass()->coerceToType(someAtom);

Note that in the last two cases, the wrapper is emitted with the proper return type, so no C++ cast is necessary.

Patch also converts a handful of places to use these, as proof-of-concept, but real client for this is mostly Flash glue code.

Two questions to ponder:
-- I pondered adding versions that take ScriptObject* (instead of Atom) but decided against it for now. Opinions?
-- these are emitted as normal C++ methods, so we rely on the linker dead-stripping the unused ones. Issue?

(Also note: the "Atom" arguments will be converted to "avm::Value" when/if that lands.)

Comment 2

[Rick Reitmaier]

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

Probably outside the scope of this bug, but as we're working on an VM interface I'd rather see a single generic mechanism for accessing any class known by the VM, rather than have multiple techniques; e.g. builtinClasses()->get_xxxClass().

Comment 3

[Steven Johnson]

(In reply to comment #2)
> Probably outside the scope of this bug, but as we're working on an VM interface
> I'd rather see a single generic mechanism for accessing any class known by the
> VM, rather than have multiple techniques; e.g.
> builtinClasses()->get_xxxClass().

That's the One True Way; the other wrappers on Toplevel are for legacy code, and will gradually be removed.

Comment 4

[Steven Johnson]

After inquiring, it turns out dead-stripping isn't reliable in all cases we need to care about:

"gcc/ld is notorious for not getting dead stripping right. Do not assume that compilers will do the right thing. In fact they do not by default. We use the '-ffunction-sections --gc-sections' hack for gcc/ld for now in Android and Linux builds but that has the downside that it impacts the optimizer and actually increases actual code size. Some more info: http://utilitybase.com/article/show/2007/04/09/225/Size+does+matter:+Optimizing+with+size+in+mind+with+GCC "

So I will revisit this with an eye towards reducing the impact.

Comment 5

[Steven Johnson]

Attachment: Patch v2

Smarter version: use stubs in ClassClosure for all, and inlines to just typecast the result type into the proper C++ type.

Comment 6

[Steven Johnson]

TR 6147:76020b2637d5

Comment 7

[Tamarin Bot]

changeset: 6148:c482f8d25cd0
user:      Steven Johnson <[e-mail]>
summary:   Bug 638956 followup: add missing explicit 'public' before isType, etc (r=me)

http://hg.mozilla.org/tamarin-redux/rev/c482f8d25cd0

Comment 8

[Tamarin Bot]

changeset: 6149:1e99d56d33a0
user:      Steven Johnson <[e-mail]>
summary:   Bug 638956 followup: add ScriptObject overloads to isType, etc (r=me)

http://hg.mozilla.org/tamarin-redux/rev/1e99d56d33a0