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

RESOLVED FIXED in Q1 12 - Brannan

Status

Tamarin
Virtual Machine
P3
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Steven Johnson, Assigned: Lars T Hansen)

Tracking

(Blocks: 1 bug)

unspecified
Q1 12 - Brannan
Dependency tree / graph

Details

(Whiteboard: PACMAN, Performance)

Attachments

(1 attachment, 1 obsolete attachment)

22.23 KB, patch
William Maddox
: review+
Edwin Smith
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

7 years ago
Blocks: 582301
(Reporter)

Updated

7 years ago
Depends on: 605324, 570608
(Assignee)

Comment 1

6 years ago
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.
Summary: Sounds we add a non-Atom version of Vector for Objects? → Should we add a non-Atom version of Vector for Objects?
(Assignee)

Comment 2

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

Updated

6 years ago
Assignee: stejohns → lhansen
(Assignee)

Comment 3

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

Comment 4

6 years ago
Created attachment 554813 [details] [diff] [review]
Work in progress
Attachment #554813 - Flags: feedback?(edwsmith)

Comment 5

6 years ago
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...
Attachment #554813 - Flags: feedback?(edwsmith) → feedback+
(Assignee)

Comment 6

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

Updated

6 years ago
Blocks: 681888
(Assignee)

Comment 7

6 years ago
Created attachment 555678 [details] [diff] [review]
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.
Attachment #554813 - Attachment is obsolete: true
Attachment #555678 - Flags: review?(edwsmith)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Flags: flashplayer-qrb?
Summary: Should we add a non-Atom version of Vector for Objects? → Specialized write path for Vector.<T> where T is known RCObject
Whiteboard: PACMAN, Performance
Target Milestone: --- → Q4 11 - Anza
(Assignee)

Updated

6 years ago
Severity: enhancement → normal

Comment 8

6 years ago
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.
Attachment #555678 - Flags: review?(wmaddox)
Attachment #555678 - Flags: review?(edwsmith)
Attachment #555678 - Flags: review+
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

6 years ago
changeset: 6568:b1b236ae99fc
user:      Lars T Hansen <lhansen@adobe.com>
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

6 years ago
(In reply to Tamarin Bot from comment #11)
> changeset: 6568:b1b236ae99fc
> user:      Lars T Hansen <lhansen@adobe.com>
> 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
....
....
....

Updated

6 years ago
QA Contact: vm → brbaker
(Assignee)

Comment 13

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

6 years ago
changeset: 6573:08cc91972163
user:      Lars T Hansen <lhansen@adobe.com>
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

6 years ago
(In reply to Tamarin Bot from comment #14)
> changeset: 6573:08cc91972163
> user:      Lars T Hansen <lhansen@adobe.com>
> 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.
(Assignee)

Comment 16

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

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

Updated

6 years ago
Target Milestone: Q4 11 - Anza → Q1 12 - Brannan
(Assignee)

Comment 18

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

Comment 19

6 years ago
Comment on attachment 555678 [details] [diff] [review]
Patch

Looks good.  R+
Attachment #555678 - Flags: review?(wmaddox) → review+
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

6 years ago
Canceling QRB request, patch landed in TR a month ago and will be in Brannan.
Flags: flashplayer-qrb?
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.