Clean up Atom definitions and manipulation code

VERIFIED FIXED in Future

Status

Tamarin
Virtual Machine
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Edwin Smith, Assigned: Edwin Smith)

Tracking

(Blocks: 1 bug)

unspecified
Future

Details

Attachments

(3 obsolete attachments)

(Assignee)

Description

9 years ago
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

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

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
(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
}
(Assignee)

Comment 4

9 years ago
Created attachment 402131 [details] [diff] [review]
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.
Assignee: nobody → edwsmith
Attachment #402131 - Flags: review?(stejohns)
(Assignee)

Updated

9 years ago
Attachment #402131 - Flags: review?(lhansen)

Updated

9 years ago
Attachment #402131 - Flags: review?(stejohns) → review+
(Assignee)

Comment 5

9 years ago
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.
(Assignee)

Comment 6

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

pushed http://hg.mozilla.org/tamarin-redux/rev/53b6e5523fd3
Attachment #402131 - Attachment is obsolete: true
Attachment #402131 - Flags: review?(lhansen)
(Assignee)

Comment 7

9 years ago
Created attachment 403309 [details] [diff] [review]
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.
Attachment #403309 - Flags: review?(stejohns)

Comment 8

9 years ago
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?
(Assignee)

Comment 9

9 years ago
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)
(Assignee)

Updated

9 years ago
Attachment #403309 - Attachment is obsolete: true
Attachment #403309 - Flags: review?(stejohns)

Updated

9 years ago
Target Milestone: --- → Future
(Assignee)

Updated

9 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 10

9 years ago
Created attachment 415920 [details] [diff] [review]
move MultinameHashtable::EMPTY into cpp file, make it Stringp instead of Atom

Better encapsulated, and doesn't need to use Atom.
Attachment #415920 - Flags: superreview?(stejohns)

Comment 11

9 years ago
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...?
(Assignee)

Comment 12

9 years ago
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.

Updated

9 years ago
Attachment #415920 - Flags: superreview?(stejohns) → superreview+

Comment 13

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

yow.
(Assignee)

Comment 14

9 years ago
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
Attachment #415920 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Blocks: 537926
(Assignee)

Comment 15

8 years ago
this seems done enough to close; further cleanup can be a new bug.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.