518157 - Clean up Atom definitions and manipulation code

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

It's currently spread between avmplusTypes.h, AtomConstants.h/cpp, and AvmCore.h/cpp, with encapsulation-breaking code in various other places.

New files proposed:
atom.h:          definitions, constants
atom-inlines.h   inline helper implementations
atom.cpp:        other helpers, constant tables

avmplusTypes.h will be gone, its only valuable contributions moved to atom.h

Comment 1

[Steven Johnson]

can we pleeeease consider making Atom into an opaque pointer type (a la Binding)?

Comment 2

[Lars T Hansen]

The time is coming soon where MMgc will need to be aware of atom tags, so if possible I would like all atom stuff completely segregated from the rest of the core stuff (ie should not make reference to AvmCore or ideally the avmplus namespace).  Separate files in the core/ directory would be fine.

Comment 3

[Edwin Smith]

(In reply to comment #2)

> The time is coming soon where MMgc will need to be aware of atom tags, so if
> possible I would like all atom stuff completely segregated from the rest of the
> core stuff (ie should not make reference to AvmCore or ideally the avmplus
> namespace).  Separate files in the core/ directory would be fine.

works for me.  namespace suggestions?  (none?)

currently we have:

namespace avmplus {
    typedef ... Atom;
    namespace AtomConstants {
        const int kObjectType = 10;
        ...
    }
}

how about:

namespace atom { 
   ... everything
}

Comment 4

[Edwin Smith]

Attachment: phase 1, move things into proper files

* consolodates avmplusTypes.h and AtomConstants.h into atom.h
* moves macro definitions to atom-inlines.h
* renames AtomConstants.cpp to atom.cpp
* adds modelines and fixes whitespace in the new files.

Comment 5

[Edwin Smith]

Macro definitions can't go in atom-inlines until many other inline functions move into their own -inlines.h files.  moving back to atom.h for now.

Comment 6

[Edwin Smith]

Comment on attachment 402131 [details] [diff] [review]
phase 1, move things into proper files

pushed http://hg.mozilla.org/tamarin-redux/rev/53b6e5523fd3

Comment 7

[Edwin Smith]

Attachment: adds cache for late bound callproperty & callproplex

One cache entry is allocated at each callproperty site with a non-runtime multiname.  The cache consists of these fields:

   handler*, vtable, slot_offset|method, Multiname*

At the call site, we invoke handler and pass a pointer to the cache along
with other call arguments.  the miss handler inspects the object or primitive,
looks up the binding, resets the handler, vtable, and data, then invokes the
handler.

for objects, handlers check that the object tag == kObjectType, and that
obj->vtable == cache->vtable.

BKIND_METHOD:  calling a method.  cache->method is set to the MethodEnv* we will call, and we just do cache->method->coerceEnter(argc, args)

BKIND_VAR|CONST: calling a function from a slot.  we compute the slot offset, load the value, and use op_call() to call it.

BKIND_NONE: calling a dynamic property.  load the value using the saved Multiname* then call with op_call.

other cases are not common and use a generic handler that calls callproperty() the old way, and resets the handler back to "miss".  (in case the call site later encounters an object/binding that has a faster handler).

Currently, we re-use cache entries for matching multinames.  There are several alternate approaches we could explore:
  * one entry per call site
  * share entries between different methods
  * put the whole multiname in the cache entry, and dont' use the precomputed multiname table in PoolObject.

Cache entries are associated with MethodInfo and the compiled code.  It is possible for a given Traits* to have multiple VTables pointing to it.  in a case like that, the cache could miss more often, when in fact the underlying Traits* (and therefore, Binding) isn't changing.  There's no evidence this is a problem yet, and caching the VTable* instead of Traits* saves at least two loads on the fast path.  (loading traits and then later loading the MethodEnv from vtable).

Cache entries are allocated from unmanaged memory, so any lingering pointers to a VTable* or MethodEnv* in the cache should not cause code to be pinned.

Comment 8

[Tom Harwood]

Comment on attachment 403309 [details] [diff] [review]
adds cache for late bound callproperty & callproplex

Looks right to me, although I did skip lightly through the latter parts.

If the caches were lazy-initialized, might we save a little space/time at leaf invocations?

Comment 9

[Edwin Smith]

the caches are lazy initialized, unless i misunderstand the question.  the handler field has to point to something before the first call, however.

(note:  i put this patch on the wrong bug! moving to bug 517762)

Comment 10

[Edwin Smith]

Attachment: move MultinameHashtable::EMPTY into cpp file, make it Stringp instead of Atom

Better encapsulated, and doesn't need to use Atom.

Comment 11

[Steven Johnson]

Comment on attachment 415920 [details] [diff] [review]
move MultinameHashtable::EMPTY into cpp file, make it Stringp instead of Atom

how can this compile? Atom and Stringp aren't compatible types...?

Comment 12

[Edwin Smith]

all the code that compares EMPTY to something, is comparing it to a Stringp.  The only way this apparently worked before was because EMPTY was Atom(0) == intptr_t(0) and the compiler is lenient about comparing pointers to 0.

with the change, EMPTY and Quad.name (what we compare EMPTY to) are both Stringp.

Comment 13

[Steven Johnson]

Comment on attachment 415920 [details] [diff] [review]
move MultinameHashtable::EMPTY into cpp file, make it Stringp instead of Atom

yow.

Comment 14

[Edwin Smith]

Comment on attachment 415920 [details] [diff] [review]
move MultinameHashtable::EMPTY into cpp file, make it Stringp instead of Atom

pushed (some time ago)
http://hg.mozilla.org/tamarin-redux/rev/d1782631ce2e

Comment 15

[Edwin Smith]

this seems done enough to close; further cleanup can be a new bug.