517762 - optimize late bound OP_callprop with inline cache

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Comment 1

[Edwin Smith]

dont inline the call to toVTable, if we dont know the base type already then do this in the helper.  

also inline fast paths in helper, and minimize boilerplate around the call site to the call helper (the late call is expensive anyway, inlining trivial ops doesn't help and just bloats the jit code).

Comment 2

[Edwin Smith]

Attachment: Move boilerplate out of jit code into callprop helper, inline fastpaths into helper

I'm not yet 100% happy with the amount of manual inlining happening here (code copying).  Will experiment with a cleaner refactoring and post again if a better approach is found.

the fast paths inlined in callprop_late are:
  * call to method binding on String or Object
  * call to function stored in Object slot
  * call to function in dynamic String or Object property

Comment 3

[Rick Reitmaier]

Comment on attachment 401847 [details] [diff] [review]
Move boilerplate out of jit code into callprop helper, inline fastpaths into helper

Nothing obviously wrong (so +'ing), but am concerned with all the cases and the testing thereof.   

Are we sure that the test suite will hit all these new code paths?

Comment 4

[Edwin Smith]

Attachment: Moves callproperty implementation to instr module.

this is just refactoring, removing code duplication, which we'll make use of in a following patch.

The reason for template <class E> is explained in instr.h.  the choices over what is REALLY_INLINED is to essentially leave the code in the same shape as before the patch.

Comment 5

[Edwin Smith]

Attachment: adds more general callprop and op_call implementations, call them from jit code

Comment 6

[Edwin Smith]

Comment on attachment 402156 [details] [diff] [review]
adds more general callprop and op_call implementations, call them from jit code

I'm not going to land this one.  the cleanup actually hurts, sometimes, and doesn't help enought to land by itself.

next up:  an inline cache for late bound calls

Comment 7

[Edwin Smith]

Comment on attachment 402152 [details] [diff] [review]
Moves callproperty implementation to instr module.

pushed 
http://hg.mozilla.org/tamarin-redux/rev/4fb2bfe42943

Comment 8

[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 9

[Steven Johnson]

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

Can we capture the bugzilla patch note as a comment in the code somewhere, so future readers of the code have a better guide as to theory of operation?

Comment 10

[Edwin Smith]

i will turn it into a design spec to be checked in.

Comment 11

[Edwin Smith]

(In reply to comment #10)
> i will turn it into a design spec to be checked in.

update: big comment added in CodegenLIR.h

Comment 12

[Edwin Smith]

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

Comment 13

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.