627920 - Should we stop using virtual methods for SO overrides?

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

There's been some thought that avoiding virtual methods in SO for special behavior would be one necessary step in providing an abstract API for the VM.

I did an audit of Flash/AIR code to see how many classes were overriding the ScriptObject methods for customizing behavior. (This ignored Proxy, which should be moved into the VM, see bug 627912). Results:


(not including Proxy, which will move into VM)

defs
0        virtual Atom getDescendants(const Multiname* name) const;
3        virtual Atom callProperty(const Multiname* name, int argc, Atom* argv);
0        virtual Atom constructProperty(const Multiname* name, int argc, Atom* argv);
4        virtual Atom getAtomProperty(Atom name) const;
0        virtual Atom getAtomPropertyFromProtoChain(Atom name, ScriptObject* protochain, Traits *origObjTraits) const;
5        virtual void setAtomProperty(Atom name, Atom value);
4        virtual bool deleteAtomProperty(Atom name);
4        virtual bool hasAtomProperty(Atom name) const;
0        virtual bool getAtomPropertyIsEnumerable(Atom name) const;
0        virtual void setAtomPropertyIsEnumerable(Atom name, bool enumerable);
2        virtual Atom getMultinameProperty(const Multiname* name) const;
2        virtual void setMultinameProperty(const Multiname* name, Atom value);
2        virtual bool deleteMultinameProperty(const Multiname* name);
2        virtual bool hasMultinameProperty(const Multiname* name) const;
3        virtual Atom getUintProperty(uint32_t i) const;
3        virtual void setUintProperty(uint32_t i, Atom value);
3        virtual bool delUintProperty(uint32_t i);
2        virtual bool hasUintProperty(uint32_t i) const;
1        virtual Atom defaultValue();        // ECMA [[DefaultValue]]
2        virtual Stringp toString();
4        virtual Atom call(int argc, Atom* argv);
lots     virtual Atom construct(int argc, Atom* argv);
4        virtual Atom nextName(int index);
4        virtual Atom nextValue(int index);
4        virtual int nextNameIndex(int index);
0        virtual Atom applyTypeArgs(int argc, Atom* argv);
0        virtual bool isMethodClosure() { return false; } // HACK?
lots        virtual ScriptObject* createInstance(VTable* ivtable, ScriptObject* prototype);
0        virtual Stringp implToString() const;
2        virtual CodeContext* getFunctionCodeContext() const { AvmAssert(0); return NULL; }
0        virtual GlobalMemoryProvider* getGlobalMemoryProvider() { AvmAssert(0); return NULL; }
0        virtual ArrayObject* toArrayObject() { return NULL; }
0        virtual uint32_t getLengthProperty();
0        virtual void setLengthProperty(uint32_t newLen);
9        virtual uint64_t bytesUsed() const;
0        virtual MethodEnv* getCallMethodEnv() { return NULL; }

With the exception of createInstance() and construct(), these are all very manageable numbers. createInstance() is defined for every native class, but arguably could and should be autogenerated with a hook for a few special cases (see bug 627192), and construct is overridden mostly to prevent construction of some classes.

Comment 1

[Lars T Hansen]

I'm very supportive.  Virtual methods are at the wrong granularity anyway; IMO the granularity you want is "protocols", which are groups of methods that must be overridden together to provide consistent behavior (eg, named property access, indexed property access, property enumeration, invocation).

Comment 2

[Edwin Smith]

+1.  I'm also willing to help with dehydra analyzers if they are useful.  a) for auditing the player code, if we need more precision than what you have done, and b) for enforcing well formed 'protocol implementation' if we can't do it with asserts.  A player/air linux build with cmake is easy to use with dehydra.

Another reason for +1 is that invoking a C++ virtual method from JIT code exposes the JIT to C++ compiler implementation hair, obscurity, and portability headaches (worst case requires wrappers).  Invoking normal functions through a function pointer is much simpler.

Comment 3

[Steven Johnson]

Attachment: Proof-of-concept sketch

This is a rough sketch of how this might look; it has plenty of rough edges but enough is here to see if people think this is going in the right direction. 

Highlights:

-- basic idea is that you can declare cls="auto", instance="auto"; doing so will cause nativegen to emit lots more stuff in the now-awkwardly named DECLARE_SLOTS macro, including declarations for *all* native methods.

-- You still can add arbitrary methods and members to your native C++ class.

-- overriding "magic" methods is now specified by metadata; we generate the proper suite of methods declarations, you provide the body.

-- It's intended that the "public API" for all this will live in avm.h; in an earlier time I was hopeful that embedders could include only this (not avmplus.h), but this seem unlikely in the short run. The goal is that you can convert a class at a time to "full auto", and convert it to use all of the new avm.h API. Obviously, the API in avm.h is incredibly skeletal and will have to be massively extended. And documented, duh.

-- Note that embedder classes now inherit from avm::Object, rather than avmplus::ScriptObject; I tried various tricks to obfuscate the inheritance further but existing pervasive mechanisms make this unlikely to be practical. Instead, I'll probably end up making sizable hunks of ScriptObject private-with-friend-to-various-VM-classes.

-- this proof of concept has converted DomainClass and ByteArray to be full "auto"; there are a number of cheats internally (ie casting from an opaque type to what we know it really is) but the concepts should be there...

-- If we go down this road, there likely will be some cruft left in during the transitory period (see ALL_CLASSES_FULL_AUTO in ScriptObject); some code internal to the VM can't really be converted to use the non-virtual-method calls directly until all subclasses that use them become full-auto. I'm not very happy about this, and am open for suggestions...

Comment 4

[Lars T Hansen]

Comment on attachment 506607 [details] [diff] [review]
Proof-of-concept sketch

Presumably virtuals on ScriptObject go away eventually (I'm looking at getAtomProperty, for example).  Or am I misunderstanding?  I see a lot of virtuals continue to live in generated code.

I am concerned that AVMTHUNK_NATIVE_CLASS_GLUE_AUTO only seems to have a variant for exactly-traced classes, there's really no reason to assume that even all avmglue classes will be exactly traced (is there?)

The change from the "multiname" terminology to the "namespaced" terminology seems unwarranted, esp since the type of the argument to hasNamespacedProperty is still "MultinameRef".

Casts, esp C-style casts, ought to be superflous in ByteArrayObject::fillInStdOverrides?  In general, can C style casts be avoided in the API layer?

Comment 5

[Steven Johnson]

(In reply to comment #4)
> Presumably virtuals on ScriptObject go away eventually (I'm looking at
> getAtomProperty, for example). 

Yep, that's the idea.

> I am concerned that AVMTHUNK_NATIVE_CLASS_GLUE_AUTO only seems to have a
> variant for exactly-traced classes, there's really no reason to assume that
> even all avmglue classes will be exactly traced (is there?)

Yep, I have fixed this in a later patch; I thought it would simplify things to require that "auto" classes are gc-exact, but it actually complicates things.
 
> The change from the "multiname" terminology to the "namespaced" terminology
> seems unwarranted, esp since the type of the argument to hasNamespacedProperty
> is still "MultinameRef".

Agreed.

> Casts, esp C-style casts, ought to be superflous in
> ByteArrayObject::fillInStdOverrides?  In general, can C style casts be avoided
> in the API layer?

the reason for the cast is that the VTable function pointers are declared using Atom (etc) while the ByteArray implementations use avm::Value (etc); we happen to know these are bit-equivalent and can legally do the cast. IMHO, as long as these casts appear *only* in machine-generated code, we're all good. I can change to C++ style casts vs C casts if you prefer.

Comment 6

[Lars T Hansen]

(In reply to comment #5)

> I can change to C++ style casts vs C casts if you prefer.

I think this would be a good rule for us to follow generally, if we trust all our compilers to provide the appropriate support.  Too often we get burned when a C-style cast that we intended to be a reinterpret_cast is interpreted by the compiler as a static_cast, with an adjusted pointer value.  Using C++ casts makes the conversions unambiguous and thus less error-prone.

Comment 7

[Edwin Smith]

Comment on attachment 506607 [details] [diff] [review]
Proof-of-concept sketch


> avm::Value (etc)

Whats the future hold for these types?  We ripped the type anonomyzers out of NativeFunction.h, but IIRC this is a different thing.  Will there be more use of them?

How worried about memory are we?  Probably, not very.  So i'll include this comment since I bothered to write it... thinking out load.

<disregard>It occurs to me that once you're below the frontier of native classes in the inheritance tree, all subclasses will have the same pointers for VTable.getNamedPropertyImpl, etc, and they all would have been in the same (single) C++ vtable.  I'm reluctant to add another load in a critical path, but just thinking out loud.  

We also could define a new (sharable) function table, and add a new ScriptObject pointer to it.  -- yet, the new SO pointer could negate any mem savings.  shrug.
</disregard>

> avm::Value

premature, but it would be good to think about members too, not just stack arguments and return values.  (ValueMember, or make sure GCRef<Value> and GCMember<Value> do the right thing.  maybe they do already?

Comment 8

[Steven Johnson]

(In reply to comment #7)
> > avm::Value (etc)
> 
> Whats the future hold for these types?  We ripped the type anonomyzers out of
> NativeFunction.h, but IIRC this is a different thing.  Will there be more use
> of them?

Very hand-wavy (and needing a bug of its own, really) but the idea here is to make an opaque type for Atom that all outside-the-VM code will use; initially it will be Atom under the hood but if we do this right then we should be able to transition to fat-box values later with minimal outside-the-VM pain.
 
> <disregard>It occurs to me that once you're below the frontier of native
> classes in the inheritance tree, all subclasses will have the same pointers for
> VTable.getNamedPropertyImpl, etc, and they all would have been in the same
> (single) C++ vtable.  I'm reluctant to add another load in a critical path, but
> just thinking out loud.  

Nah, this is a valid concern, actually; we don't actually add a new load (as once everything is converted we'd remove the C++ vmethods and always use the VTable ptr) but I guess you're saying we could conserve the space by adding another indirection... bleah. Alternately... we could keep the actual implementation of these as virtual methods (C++ style) under the hood, but have nativegen be in charge of how they get emitted...
 
> > avm::Value
> 
> premature, but it would be good to think about members too, not just stack
> arguments and return values.  (ValueMember, or make sure GCRef<Value> and
> GCMember<Value> do the right thing.  maybe they do already?

GCRef<Value> etc don't do the right thing now, but arguably they should. But yeah, we need a Member variant.

Comment 9

[Steven Johnson]

(In reply to comment #8)

> Very hand-wavy (and needing a bug of its own, really) but the idea here is to
> make an opaque type for Atom that all outside-the-VM code will use; initially
> it will be Atom under the hood but if we do this right then we should be able
> to transition to fat-box values later with minimal outside-the-VM pain.

https://bugzilla.mozilla.org/show_bug.cgi?id=629746

Comment 10

[Steven Johnson]

I think I will withdraw this bug in favor of https://bugzilla.mozilla.org/show_bug.cgi?id=629757, as the question of changing the calling mechanism will be much simpler to evaluate with that work in place.

Comment 11

[Rick Reitmaier]

Nice work looking forward to further patches...along with comments above, just a small note regarding the xxxRef usage in avm.h.   We probably want to expose toRef(xxx) type methods to do the conversion; ignore, if they are already there.