601817 - Specialized write path for Vector.<T> where T is known RCObject

Status: RESOLVED FIXED

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

Description

[Steven Johnson]

ObjectVectorObject is really an Atom-based list; this is fine for Objects, but for specializations that aren't Vector<*> or Vector<Object>, we could have more efficient code by having an implementation that uses ScriptObject* instead of Atom. (This could be done very cheaply if/when bug 570608 lands.)

Comment 1

[Lars T Hansen]

This would save generated code from doing tag insertion and tag stripping, and would make GC scanning of the vectors faster (no tag sniffing).

If vectors are generally used in typed contexts it would benefit us; if they are generally used in untyped contexts I'm less sure.  But I'm guessing they're generally used in typed contexts and the fix seems like it ought to be a win.

Comment 2

[Lars T Hansen]

Tag sniffing is significant also during write operations of course: with an Atom storage type, we have to go through the atomWriteBarrier which is more expensive than WBRC: at least four ALU instructions and a conditional branch for both the old value and the new value, in addition to the RC manipulations.

Comment 3

[Lars T Hansen]

Initial experiments with a change that keeps using AtomList and Atom representations for the vector traffi, but which introduces a simple fast path for when the vector is known to contain only RCPointer values and the value stored is a known RCPointer, shows nearly a 50% speedup on the "write object to vector" benchmark (see bug #599099): from 1857ms to 1263ms.

The speedup comes from avoiding the type switching in atomWriteBarrier and also inlining what remains of atomWriteBarrier into the specialized jit callout, thereby minimizing calling overhead.  The resulting code is still too large to in-line in the jitted code at this time.

Comment 4

[Lars T Hansen]

Attachment: Work in progress

Comment 5

[Edwin Smith]

Comment on attachment 554813 [details] [diff] [review]
Work in progress

Nothing obviously bad.  the speedups sound worthwhile.

The more we add special helpers like this, the more I wonder if we should treat int separately - always inline the <0 test (or combine paths with uint by using the uint-cmp trick), then cut the # of helpers in half, only support uint indexes.  other ideas welcome...

Comment 6

[Lars T Hansen]

(In reply to Edwin Smith from comment #5)

> The more we add special helpers like this, the more I wonder if we should
> treat int separately - always inline the <0 test (or combine paths with uint
> by using the uint-cmp trick), then cut the # of helpers in half, only
> support uint indexes.  other ideas welcome...

This is attractive, it would ease our maintenance burden by a bit.  With this patch I'm probably more or less done adding specialized vector helpers, but it's worth keeping in mind as we attack other data structures.

Comment 7

[Lars T Hansen]

Attachment: Patch

This is cleaner, has a more robust test for RCObject-ness (the old test was incorrect notably for Boolean), and includes an additional performance test case for Vector.<Boolean>.

Running this (32-bit Mac Pro Xeon), I see a score improvement on vector-write-C* from about 54 to about 80, ie, a speedup of 1.48.  As expected, vector-write-Object* stays the same, since Object fails the "known RCObject" test.

Comment 8

[Edwin Smith]

Comment on attachment 555678 [details] [diff] [review]
Patch

Looks right (and clean!) but asking for a second review (not to block landing).

while-youre-in-there: 

Traits::isRCObjectSlot(), isAtomOrRCObjectSlot(), and maybe others could be static.

Comment 9

[Lars T Hansen]

(In reply to Edwin Smith from comment #8)

> while-youre-in-there: 
> 
> Traits::isRCObjectSlot(), isAtomOrRCObjectSlot(), and maybe others could be
> static.

Those are not actually methods on Traits, but global functions.

Comment 10

[Lars T Hansen]

Pushed, but keeping the bug open until review from wmaddox is in.

Comment 11

[Tamarin Bot]

changeset: 6568:b1b236ae99fc
user:      Lars T Hansen <[e-mail]>
summary:   Fix 601817 - Specialized write path for Vector.<T> where T is known RCObject (r=edwsmith)

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

Comment 12

[Brent Baker]

(In reply to Tamarin Bot from comment #11)
> changeset: 6568:b1b236ae99fc
> user:      Lars T Hansen <[e-mail]>
> summary:   Fix 601817 - Specialized write path for Vector.<T> where T is
> known RCObject (r=edwsmith)
> 
> http://hg.mozilla.org/tamarin-redux/rev/b1b236ae99fc

This revision is causing the following assert to happen when OSR is enabled:
../configure.py --enable-debug --enable-osr

./avmshell as3/Vector/reverse.abc
Assertion failed: "((oldtag == kObjectType || oldtag == kStringType || oldtag == kNamespaceType))" ("../core/avmplusList-inlines.h":316)

#0  0x00007fff825330b6 in __kill ()
#1  0x0000000100007cd5 in VMPI_debugBreak () at ../VMPI/MacDebugUtils.cpp:55
#2  0x0000000100007ca7 in avmplus::AvmDebugMsg (p=0x1001e7810 "Assertion failed: \"((oldtag == kObjectType || oldtag == kStringType || oldtag == kNamespaceType))\" (\"../core/avmplusList-inlines.h\":316)\n", debugBreak=true) at ../AVMPI/AvmAssert.cpp:69
#3  0x0000000100006a22 in avmplus::AvmAssertFail (message=0x1001e7810 "Assertion failed: \"((oldtag == kObjectType || oldtag == kStringType || oldtag == kNamespaceType))\" (\"../core/avmplusList-inlines.h\":316)\n") at AvmAssert.h:66
#4  0x0000000100006a42 in avmplus::_AvmAssertMsg (condition=0, message=0x1001e7810 "Assertion failed: \"((oldtag == kObjectType || oldtag == kStringType || oldtag == kNamespaceType))\" (\"../core/avmplusList-inlines.h\":316)\n") at AvmAssert.h:72
#5  0x00000001000c26ba in avmplus::AtomListHelper::storePointer (data=0x101185a68, index=0, value=4312414026) at avmplusList-inlines.h:316
#6  0x00000001000c27c3 in avmplus::AtomList::setPointer (this=0x1011dc3d8, index=0, value=4312414026) at avmplusList-inlines.h:283
#7  0x00000001000c2860 in avmplus::ObjectVectorObject::_setKnownIntPropertyWithPointer (this=0x1011dc3a8, index_i=0, value=4312414026) at VectorClass.h:528
#8  0x0000000101257e3a in ?? ()
#9  0x00000001000d17b0 in avmplus::BaseExecMgr::endCoerce (env=0x101191d88, argc=0, ap=0x7fff5fbfea20, ms=0x101190648) at ../core/exec.cpp:727
#10 0x00000001000d68d6 in avmplus::OSR::execute (env=0x101191d88, interp_frame=0x7fff5fbfea20, ms=0x101190648, osr_pc=0x1011a09e8 "\tdl?\001dl?\001dl?\001a\004dl?\001???d+m?\001\b\002dl?\001$\024\025???dl?\001F?\001", result=0x7fff5fbfee78) at ../core/exec-osr.cpp:212
#11 0x00000001000dde5b in avmplus::interpBoxed (env=0x101191d88, _argc=0, _atomv=0x7fff5fbff0f0) at ../core/Interpreter.cpp:1889
....
....
....

Comment 13

[Lars T Hansen]

The assert is too aggressive.  If the slot is previously uninitialized it will contain all-zero-bits and not be properly tagged; the subsequent code handles this just fine but the assert does not.

Comment 14

[Tamarin Bot]

changeset: 6573:08cc91972163
user:      Lars T Hansen <[e-mail]>
summary:   For 601817 - Specialized write path for Vector.<T> where T is known RCObject: fix too-aggressive assert (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/08cc91972163

Comment 15

[Brent Baker]

(In reply to Tamarin Bot from comment #14)
> changeset: 6573:08cc91972163
> user:      Lars T Hansen <[e-mail]>
> summary:   For 601817 - Specialized write path for Vector.<T> where T is
> known RCObject: fix too-aggressive assert (r=lhansen)
> 
> http://hg.mozilla.org/tamarin-redux/rev/08cc91972163

Confirmed that the OSR debug build no longer triggers the assert.

Comment 16

[Lars T Hansen]

BTW, OSR has nothing to do with it - the issue is that OSR will JIT code that was previously only interpreted except in -Ojit mode (top-level code).  The test case in question uses vectors in top-level contexts.

That's a big argument in favor of running -Ojit builds in Debug mode to flush out more bugs like these.  Brent is seeing to that as we speak.

Comment 17

[Brent Baker]

(In reply to Lars T Hansen from comment #16)
> That's a big argument in favor of running -Ojit builds in Debug mode to
> flush out more bugs like these.  Brent is seeing to that as we speak.

Logged as bug# 686285

Comment 18

[Lars T Hansen]

All patches landed, bug is just waiting on extra review from Bill.

Comment 19

[William Maddox]

Comment on attachment 555678 [details] [diff] [review]
Patch

Looks good.  R+

Comment 20

[Lars T Hansen]

Canceling QRB request, patch landed in TR a month ago and will be in Brannan.