609294 - Need a story around Atom for safegc

Status: VERIFIED DUPLICATE of bug 629746

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

Description

[Tommy Reilly]

For Atom fields we need a HeapAtom class.  The existing avmplus::AtomWB class serves this purposes, we should just eradicate the ATOM_WB macro and rename it HeapAtom (parity with Multiname approach).

For Atom stack references I think we need an AtomRef class or somesuch which is a real class which has rules like GCRef (has to be used on stack not heap, can't be used to obtain pointers, can return ScriptObjectRef/StringRef etc).

Comment 1

[Tommy Reilly]

Requirements: 

1) Raw Atom type should be hidden from player/shell
2) It should be a compile error to put a HeapAtom class in anything other than a GCRoot/GCObject/GCFinalizedObject
3) It should be an assert (don't think compile time it possible) error to put an AtomRef in any other than the stack or a stack only allocated object

Feedback requested:

Are HeapAtom and AtomRef acceptable names?   They would live in the avmplus namespace.

Should AtomRef be an Atom in release builds and a class in debug builds (worried about performance, premature?)

Comment 2

[Lars T Hansen]

Could we simply use GCMember<Atom> and GCRef<Atom>?  I know there's a bit of overloading of conceptual machinery, but it seems that fewer names are better than more names.

I'm sure that will run afoul of some existing nefarious uses of Atom in the Flash Player code base though...

I'm not on board with HeapAtom - HeapHashtable is not a good name.

Comment 3

[Tommy Reilly]

GCMember and GCRef assume T is a class, specifically one derived from GCObject or GCFinalizedObject.   And the storage is set up as T* which isn't what we want.

We have the Heap prefix in multiple places and its spreading, HeapMultiname, HeapList, HeapAtomList, HeapHashtable, HeapE4XNodeList, HeapNamespaceList.   

avmplus::AtomMember and avmplus::AtomRef?

Comment 4

[Lars T Hansen]

I could absolutely live with AtomMember and AtomRef.  They should presumably be in the same namespace as GCMember and GCRef, and all four should normally be available in the player without further qualification (IMO).

Comment 5

[Edwin Smith]

Is it impossible to use garden variety template specialization for GCMember<Atom> (etc) because of the type sniffing that we do?

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #3)
> GCMember and GCRef assume T is a class, specifically one derived from GCObject
> or GCFinalizedObject.   And the storage is set up as T* which isn't what we
> want.

I took comment 2 to mean that we would introduce a distinct template
specialization (and a *complete* specialization) for GCMember<Atom> and
GCRef<Atom> that would not reuse the machinery in GCMember<T> or GCRef<T>, so
it would not be setting up the storage in the same way.  Assuming its feasible
to set up such a specialization.

(I'm not saying I like the conceptual overloading; it just seemed like there
was miscommunication between comments 2 and 3. . .)

Comment 7

[Tommy Reilly]

My bad, I can't think of any reason GCMember<Atom> and GCRef<Atom> wouldn't work with complete template specialization.     My template foo is weak ;-)

Comment 8

[Steven Johnson]

I could live with either AtomMember+AtomRef, or GCMember<Atom>+GCRef<Atom>.

Better yet, make both acceptable.

Comment 9

[Steven Johnson]

But for GCMember<Atom> to work, have to change Atom to be a unique type, not a uintptr. (Needs to be done anyway, it's just more work.)

Comment 10

[Tommy Reilly]

(In reply to comment #9)
> But for GCMember<Atom> to work, have to change Atom to be a unique type, not a
> uintptr. (Needs to be done anyway, it's just more work.)

Why?  I thought we had established that we could specialize GCMember<Atom> to avoid any dependencies on it being a type.  We want to change Atom to be a real class yes, but I don't think doing that needs to block the change to GCMember<Atom>.

Comment 11

[Steven Johnson]

templates can't specialize on a typedef; since "typedef intptr_t Atom", Atom looks like intptr_t to the type system, which means you could also do GCMember<int32_t>, which would be bad

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #11)
> templates can't specialize on a typedef; since "typedef intptr_t Atom", Atom
> looks like intptr_t to the type system, which means you could also do
> GCMember<int32_t>, which would be bad

I had assumed that such usage would, in the meantime, be caught during code reviews.  But perhaps that is assuming too much.  So maybe we should just go to AtomMember and AtomRef for now.

Comment 13

[Steven Johnson]

(In reply to comment #12)
> I had assumed that such usage would, in the meantime, be caught during code
> reviews.  

That would be nice; also, I would like a pony. (When it comes to glue code, assume that if it *can* be misused, it *will* be misused)

Comment 15

[Trevor Baker]

bulk verifying resolved !fixed issues