639613 - ANI: provide a cleaner way to access utilities in AvmCore/Toplevel

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Steven Johnson]

Right now, native classes must use a grab bag of code in AvmCore and Toplevel; this should be rationalized and exposed to native code in a clean way, avoiding exposure of unnecessary detail.

Comment 1

[Steven Johnson]

Attachment: Patch 1, v1

The central concept here is that eventually, AvmCore and Toplevel should be implementation details that aren't visible to 99% of code outside the VM; further down the road, there should be no need to subclass either one.

Instead, we provide a new, deliberately extensible structure: "avm::Info". This has effectively the same lifespan as a Toplevel, but is designed to expose functionality in a more granular way; instead of lumping random collections of calls together, the avm::Info is really just an aggregator of sub-managers. It will definitely hold builtinClasses, string constants the Value manager for ANI, and probably other things... basically, anything that requires AvmCore or Toplevel now (and can't be done equally well in some other way) will be moved into this new structure.

Also note that it's designed to be extended; see ShellInfo for a trivial example. Flash/AIR will make much greater use of this facility (eg for Player-specific string constants).

Note: code inside the VM proper is *not* required to use this structure (rather than AvmCore/Toplevel); the real goal is to provide a maintainable, public-facing API. I'd suggest looking at the usage in SystemClass.cpp to see what likely future glue code is likely to look like.

Note: I'm open for other naming suggestions; I consider "Info" to be adequate but suboptimal.

Comment 2

[Steven Johnson]

Attachment: Patch 2 v1

Augment avm::Info further by adding factory methods to StringClass. The idea here is that since StringClass is now easily accessible, let's have the factory methods live there rather than AvmCore.

Comment 3

[Steven Johnson]

Attachment: Patch 3, v1

Move all of the string constants (cached chars and well-known strings) out of AvmCore and into their own structures; provide access via avm::Info. (Note that access to stringConstants is also available "inside the VM" directly from AvmCore, but cachedChars is now private.)

It might make sense to move cachedChars to StringClass in the future.

This is a high-churn change but it moves a lot of stuff out of a GCRoot that never really needed to be there in the first place. (It also demonstrates the approach that I want to use for Flash strings constants.)

Comment 4

[Steven Johnson]

OK, that's enough to see the general concept I think, though these examples don't show many "interesting" additions via ShellInfo, I think you can figure out the idea. Opinions on the general approach welcomed; opinions on specific nomenclature also welcomed.

Comment 5

[Steven Johnson]

Comment on attachment 517625 [details] [diff] [review]
Patch 3, v1

(Adding treilly to review the TraceLocations I added, since there wasn't an array-of-GCMember option)

Comment 6

[Steven Johnson]

Comment on attachment 517625 [details] [diff] [review]
Patch 3, v1

(Adding treilly to review the TraceLocations I added, since there wasn't an
array-of-GCMember option)

Comment 7

[Jeff Mott]

Overall it sounds good.

Comment 8

[Steven Johnson]

No other comments? Anyone?

Comment 9

[Edwin Smith]

My initial reaction is I don't like "Info", because the concept doesn't seem crisp; something concrete like "AVM*" (the whole vm) or "Environment*" (the whole enviornment) makes more sense to me.  I'd like to keep the F? pending until i have time to think more about it (soon).

Comment 10

[Steven Johnson]

I'm definitely not married to "Info" as a name. The general concept is what I really want feedback on.

Comment 11

[Edwin Smith]

my point was that the general concept seems fuzzy to me, and its reflected by a fuzzy name.  need to study more, but i'm hoping for something more concrete (concept wise) which leads to an obvious name.

Comment 12

[Steven Johnson]

Today's thought, perhaps "avm::Instance" is a more useful name.

Comment 13

[Rick Reitmaier]

Comment on attachment 517619 [details] [diff] [review]
Patch 2 v1

Confusion might exist while we have both sets of methods available.  Is there a bug open to track the removal of the existing avmcore based variants?

Comment 14

[Rick Reitmaier]

Comment on attachment 517625 [details] [diff] [review]
Patch 3, v1

r+ agree in principal and have lightly review individual changes...assuming that cut and paste went well.   

If you're looking for a deeper review then we probably want at least one other set of eyes on this (besides Tommy), as the changes are quite pervasive.

Comment 15

[Edwin Smith]

Comment on attachment 517618 [details] [diff] [review]
Patch 1, v1

Dropping the feedback? flag until something more current is posted.   Generally going in the right direction, I think.

Comment 16

[Ruchi Lohani]

Attachment: Patch 1, v2

Comment 17

[Ruchi Lohani]

Comment on attachment 555470 [details] [diff] [review]
Patch 1, v2

Feedback request on updated patch.

Comment 18

[Rick Reitmaier]

Comment on attachment 555470 [details] [diff] [review]
Patch 1, v2

Review of attachment 555470 [details] [diff] [review]:
-----------------------------------------------------------------

+'ing as there is no obvious defects.  But I'm more curious where we're planning on taking this effort.

If it fits in with our overall goals for ANI then fine, but if we're landing it simply because its lingering and we'd like to clear it off the table, then I object.

Said another this looks like part I of a 2 part approach to cleaning up this area.  I just want to make sure we do part 2.

::: utils/nativegen.py
@@ +1878,4 @@
>          out.indent += 1
>          for as3name,cppname,clsid in names:
>              # We can't use static_cast<> because the subclass is only forward-declared at this point
> +            out.println("REALLY_INLINE GCRef<%s> get_%s() const { return (%s*)(lazyInitClass(%s)); }" % (cppname, as3name, cppname, clsid))

is this a whitespace change?