635979 - ANI: provide better support for getting "builtin" classes

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Currently, getting access to an AS3 Class object in C++ is awkward; some builtin classes are surfaced as public variables in Toplevel, some by accessor functions, some by getBuiltinClass() or getBuiltinExtensionClass(); Flash/AIR has its own scheme.

Instead, let's replace this with a type-safe class-manifest accessor; this could be generated by nativegen and replace all the ad-hoc stuff with a predictable (and cast-free) mechanism.

Comment 1

[Steven Johnson]

Attachment: Part 1: nuke getBuiltinExtensionClass

The only thing this virtual method does is allow SamplerScript to get a few classes it has to construct to create samples. Instead, let's just special-case this to pass in the classclosures we need from AS3. (This is not really an ideal solution, frankly, but SamplerScript is unique in that it has to live in both avmshell and Flash/AIR, so can't know about its Toplevel situation; IMHO we should proabably move Sampler out of "extensions" but that's outside the scope of this bug.)

Comment 2

[Steven Johnson]

Attachment: Part 2: nativegen.py emits a well-typed ClassManifest

Comment 3

[Steven Johnson]

Attachment: Part 3: remove getBuiltinClass(), use builtinClasses() in the VM instead

Fair amount of churn, but ensuring that all code went thru accessors rather than grabbing data fields directly was worthwhile (with exception of objectClass, for now)

Comment 4

[Steven Johnson]

Attachment: Part 4: convert ShellCore to use similar approach

(Note that ShellCore was only using this to provide to SamplerScript, which we handled in a different way in the first patch, but this is useful as a proof of concept.)

Comment 5

[Steven Johnson]

Attachment: Part 5: add one more API that Flash needs

(Note that it's possible for Flash to do this itself, but encapsulation is good)

Comment 6

[Christopher A. Brichford]

(In reply to comment #1)
> Created attachment 514289 [details] [diff] [review]
> Part 1: nuke getBuiltinExtensionClass
> 

I think the ClassFactory class in Sampler.as should be marked as FP_SYS and/or AIR_SYS.  Further that class' native meta-data should say that it can't be constructed.

Comment 7

[Steven Johnson]

(In reply to comment #6)
> I think the ClassFactory class in Sampler.as should be marked as FP_SYS and/or
> AIR_SYS.  Further that class' native meta-data should say that it can't be
> constructed.

VM_INTERNAL is the new AIR_SYS, but yeah, good call. Will do.

Comment 8

[Rick Reitmaier]

Comment on attachment 514369 [details] [diff] [review]
Part 4: convert ShellCore to use similar approach

r+ with a few comments :
- not sure why prepareActionPool() is in comments.
- what is rationale for using REALLY_INLINE in this case? Not directed at this specific change but just a general comment; My gut feeling is that we're abusing this mechanism and its leading to poor code generation.  At the very least its introducing enough instability in our performance numbers to make profiling changes extremely difficult.  I'll have to back it up with numbers.
- prepareBuiltinActionPool appears to mostly duplicate the code in handleActionPool; could a refactoring help?

Comment 9

[Steven Johnson]

(In reply to comment #8)
> Comment on attachment 514369 [details] [diff] [review]
> - not sure why prepareActionPool() is in comments.

Because I am a sloppy coder, oops :-/

> - what is rationale for using REALLY_INLINE in this case? 

There is none, really; it needs to be in the header file due its templated nature, and I was sloppy. Will de-inline.

> - prepareBuiltinActionPool appears to mostly duplicate the code in
> handleActionPool; could a refactoring help?

Yeah, really, all of the parse/prepare/handleActionBlock/Pool functions need cleanup and scouring. I hope to do that in a later phase.

Comment 10

[Rick Reitmaier]

Comment on attachment 514289 [details] [diff] [review]
Part 1: nuke getBuiltinExtensionClass

much nicer.

Comment 11

[Rick Reitmaier]

Comment on attachment 514368 [details] [diff] [review]
Part 3: remove getBuiltinClass(), use builtinClasses() in the VM instead

r+ ;

- VectorClass; might be worth mentioning the bug # in a comment associated with said 'ugly hack'.

- Would be nice to have some comments around the whole class initialization process.  In particular what/why lazyIntClass is doing.  Maybe there are and I just missed them .

- I trust that their are no other circular dependencies, such as in MethodEnv.newFunction, wherein we can't use the accessor; is there a way to guarantee it, besides inspection?

Comment 12

[Steven Johnson]

Attachment: Part 6: followup fixes

various minor fixes suggested in the comments, plus one susbtantive bug fix: the class claimed to be GCExact, but neglected to trace _env, making for fun hard to track down bugs. Fixed that and refactored ClassManifestBase to move more functionality into the base class to simplify things.

Comment 13

[Steven Johnson]

pushed all to TR (in advance of final review, with rreitmai's permission), as 5974:2fddec8f5451

Comment 14

[Tamarin Bot]

changeset: 5975:d7259e8e4c87
user:      Steven Johnson <[e-mail]>
summary:   Bug 635979 followup: add bool!=0 love for MSVC to avoid conversion complaints (r=stejohns)

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

Comment 15

[Tamarin Bot]

changeset: 5976:72371ecb42eb
user:      Steven Johnson <[e-mail]>
summary:   Bug 635979 followup: add explicit namespacing to appease MSVC (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/72371ecb42eb