733807 - Use a per-abc-env finddef table indexed by multiname id to speed up OP_finddef.

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

Description

[Edwin Smith]

Because of the rules that allow DomainEnv lookups to be sound, once finddef returns an object for a given name, for any reference coming from within the same DomainEnv (and by proxy, the same AbcEnv), finddef always returns the same object, even if global objects mutate and even if new content is loaded.

So, can implement a fastpath for finddef using a per-AbcEnv table indexed by nameid.

This table can be used by the interpreter and JIT, for OP_finddef.  Once this is done the old lookupTimestamp and MethodEnv lookup_cache is no longer useful and can be removed.

Comment 1

[Edwin Smith]

Its actually easy to inline the fast path for this, but doing so introduces an if/else branch, which prevents dead code elimination from removing the unused finddef.

Comment 2

[Edwin Smith]

Attachment: Add and use FinddefTable in JIT and Interpreter

Comment 3

[Edwin Smith]

Attachment: remove vestigal LookupCache

This patch removes the timestamp-based lookup_cache, which now was only being used by wordcode interpreter for dynamic global object refs (not finddef), which are rare-to-never seen in AS3.

Comment 4

[William Maddox]

Comment on attachment 603759 [details] [diff] [review]
Add and use FinddefTable in JIT and Interpreter

Review of attachment 603759 [details] [diff] [review]:
-----------------------------------------------------------------

R+.  I'm taking your word for it that all invocations of finddef must return the same value, as I don't understand the DomainMgr very well.  But I presume that a Domain is similar to a
Java classloader, and all bets are off with respect to type safety if we can't assume that names have a consistent meaning there.

::: core/CodegenLIR.cpp
@@ +3302,4 @@
>              int32_t dest_index = state->sp() + 1;
> +            LIns* table = loadEnvFinddefTable();
> +            // We always call out here because inlining the fast path inhibits
> +            // dead code limination when this finddef isn't used.

limination -> elimination

::: core/instr.cpp
@@ +197,5 @@
> +    ScriptObject** obj0_ptr = (ScriptObject**) &table[0];
> +    int name_id = int(obj_ptr - obj0_ptr);
> +    // execute finddef and save the result
> +    ScriptObject* obj = env->finddef(env->method->pool()->precomputedMultiname(name_id));
> +    table[name_id].object = obj;

I presume you broke this out so that findef_cache could avoid allocating stack for locals and/or temps to support the more complicated miss case.  This needs a comment, since it doesn't look like finddef_miss is called from anywhere else.

Comment 5

[Tamarin Bot]

changeset: 7263:855c94bab81d
user:      Edwin Smith <[e-mail]>
summary:   Bug 733807 - Use a per-abc-env finddef table indexed by multiname id to speed up OP_finddef

http://hg.mozilla.org/tamarin-redux/rev/855c94bab81d

Comment 6

[Tamarin Bot]

changeset: 7264:818ad1c7d486
user:      Edwin Smith <[e-mail]>
summary:   Bug 733807 - Use a per-abc-env finddef table indexed by multiname id to speed up OP_finddef. (r=wmaddox+)

http://hg.mozilla.org/tamarin-redux/rev/818ad1c7d486

Comment 7

[Tamarin Bot]

changeset: 7265:c7d87b451857
user:      Edwin Smith <[e-mail]>
summary:   Bug 733807 - Remove LookupCacheBuilder and LookupCache

http://hg.mozilla.org/tamarin-redux/rev/c7d87b451857

Comment 8

[Brent Baker]

Patches "Use a per-abc-env finddef table" are causing the following assertion when running a float enabled shell.

...
#6  0x000022a7 in avmplus::_AvmAssertMsg (condition=0, message=0x22d6c8 "Can't call FindBeginning on something pointing to GC header") at AvmAssert.h:72
#7  0x00062a16 in MMgc::GCLargeAlloc::FindBeginning (item=0x10e9000) at GCLargeAlloc-inlines.h:97
#8  0x00062a92 in MMgc::GC::FindBeginningFast (this=0x1005018, gcItem=0x10e9000) at GC-inlines.h:533
#9  0x00209660 in MMgc::GC::InlineWriteBarrierGuardedTrap (this=0x1005018, address=0x10ea000) at WriteBarrier-inlines.h:147
#10 0x0020a3b0 in MMgc::GC::InlineWriteBarrierRC (address=0x10ea000, value=0x10f3110) at WriteBarrier-inlines.h:113
#11 0x002030e0 in MMgc::GC::WriteBarrierRC (address=0x10ea000, value=0x10f3110) at ../MMgc/GC.cpp:3138
#12 0x00157645 in MMgc::RCObject::WriteBarrier (address=0x10ea000, value=0x10f3110) at GCObject.h:667
#13 0x000a4e90 in MMgc::GCMemberBase<avmplus::ScriptObject>::set (this=0x10ea000, tNew=0x10f3110) at GCMember-inlines.h:58
#14 0x001576ae in MMgc::GCMemberBase<avmplus::ScriptObject>::operator= (this=0x10ea000, tNew=0x10f3110) at GCMember-inlines.h:130
#15 0x000a4eac in MMgc::GCTraceableBase::GCMember<avmplus::ScriptObject>::operator= (this=0x10ea000, tNew=0x10f3110) at GCMember-inlines.h:324
#16 0x00196196 in avmplus::finddef_miss (obj_ptr=0x10ea000, frame=0xbfffe838) at ../core/instr.cpp:201
#17 0x001961c8 in avmplus::finddef_cache (obj_ptr=0x10ea000, frame=0xbfffe838) at ../core/instr.cpp:214
#18 0x01152fa9 in ?? ()
#19 0x000f4790 in avmplus::BaseExecMgr::endCoerce (env=0x1166080, argc=1, ap=0xbfffe930, ms=0x1164b90) at ../core/exec.cpp:868
...
...

This happens only reproduced with 32bit builds when running:
as3/Vector/Vector_double_methods.abc

More information can be provided if needed on how to build a float enabled shell and test media.

Comment 9

[Edwin Smith]

Attachment: Use a manual write barrier when updating FinddefTable

This fixes the test case and looks more correct to me.  Could you take a look at how AbcEnv::m_finddef_table is declared to make sure I'm using ExactStructContainer correctly?

Basically I just want m_finddef_table to be an array of exactly traced ScriptObject* pointers.

Comment 10

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

Aesthetically I'd prefer to keep the GCMember version if we can.

I cannot yet tell from the stack trace from comment 8 whether the firing assertion is discovering a bug in FindBeginning, or a bug in how this object is allocated, or some broader bug in the heap's structural invariants, or something exceptional about this particular data structure that allows it to be, well, an exception.

I'll keep looking.

Comment 11

[Edwin Smith]

The intention was for AbcEnv::m_finddef_table to simply be a fixed-sized array of ScriptObject* pointers, allocated once when AbcEnv is allocated.  I did my best to copy the pattern from PoolObject::precompNames.

The reason I didn't choose RCList for this is because I want array elements to be easy to access from JIT code and RCList's internal array was too indirect for my taste.

Comment 12

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

Some initial notes from skimming the patch and (from what I see) the only other use of ExactStructContainer:

* If possible, I would make FinddefEntry extend GCInlineObject, the same way that HeapMultiname does.  (I am a little disturbed that its use of GCMember<> worked at all.  I guess this is probably a case of our little SafeGC puns biting us here.)

* From reviewing HeapMultiname: even that code uses explicit write-barriers rather than using GCMember, and the reason for this is documented on Bug 525875.  (So my desire for sticking with the GCMember variant is a little weak.  Nonetheless, I want to understand what is going wrong here, why GCMember is both not working properly but also not immediately failing loudly.)

-- Summary of Bug 525875: there was once some serious unsoundness when doing GetGC() on large offsets into a given object.

-- and so we would work around that unsoundness by using manual write-barriers when the offset might be large (rather than incur the cost of the most general write barrier in all cases).

-- (We did eventually change the api to avoid passing large offsets to the GetGC() function, namely by making the WriteBarrier, if possible, use the referred-object as the basis for the extraction rather than than the referrer itself.)

-- So: Bug 525875 does not itself explain this particular bug.  But like this bug, it is a case where we show weakness in testing behavior at large offsets (or at least "interesting" large offsets like multiples of the block size, which seems to be our problem here.).

* Anyway, Ed's patch seems sound, in isolation.  R+.

* (I'm just worried that if we find ourselves requiring sprinklings of write-barriers in our code here, how on earth can we be confident about the uses of GCMember in the rest of the code base?)

Comment 13

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

(In reply to Felix S Klock II from comment #12)
> we show weakness in testing behavior at large
> offsets (or at least "interesting" large offsets like multiples of the block
> size, which seems to be our problem here.).

Filed as followup work item: Bug 735121.

Comment 14

[Tamarin Bot]

changeset: 7273:50d9eebf86ad
user:      Edwin Smith <[e-mail]>
summary:   Bug 733807 - Use a manual WB to avoid findBeginning madness. (pending r=fklockii?)

http://hg.mozilla.org/tamarin-redux/rev/50d9eebf86ad

Comment 15

[Edwin Smith]

(In reply to Felix S Klock II from comment #12)

> * If possible, I would make FinddefEntry extend GCInlineObject, the same way
> that HeapMultiname does.  (I am a little disturbed that its use of
> GCMember<> worked at all.  I guess this is probably a case of our little
> SafeGC puns biting us here.)

> * (I'm just worried that if we find ourselves requiring sprinklings of
> write-barriers in our code here, how on earth can we be confident about the
> uses of GCMember in the rest of the code base?)

Agreed.  Do you think its worth trying to go back to GCMember (with FinddefEntry extending GCInlineObject) now, or better to wait until we investigate findBeginning (Bug 735121)?

Comment 16

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

(In reply to Edwin Smith from comment #15)
> Agreed.  Do you think its worth trying to go back to GCMember (with
> FinddefEntry extending GCInlineObject) now, or better to wait until we
> investigate findBeginning (Bug 735121)?

I already tried a cursory investigation (of going back to the GCMember version, changing it to GCInlineObject, and seeing what happened).  Things still break.  Lets keep the manual WB version you have put here, and I will continue investigation of the problem on my end (which seems like a more general GC issue that may or may not be linked to some nasty exactgc-tracer crashes I've seen trickling in via Watson).

Comment 17

[Edwin Smith]

Ok, closing.