583275 - remove unnecessary unboxing cost of obj->getPrivate() on x64

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

A "private pointer" only requires that the low bit of the given void* is unset so, on x64, the simplest way to produce a GC-safe value is to >>1 to produce a double and <<1 to unbox.  This means obj->getPrivate(), which is very hot, acquired an extra shift with x64 fatvals.  This is unnecessary since fslot[JSSLOT_PRIVATE] is specifically not marked.  I'd like to do:

union {
  void *privatePtr;
  fslots[JS_INITIAL_NSLOTS];
};

and split the JSObject::init() and initSharingEmptyScope() functions to either take a void* or the Value for the (non-private) first fslot.  This avoids copying a word on object-init for x86.

obj->getPrivate is really hot (also on trace), so I would expect a couple ms boost on x64.

I suspect this whole area is about to be shaken up by bug 558451, so blocking on that.

Comment 1

[Andreas Gal :gal]

What reads the privatePtr so much?

Comment 2

[Luke Wagner [:luke]]

Its on the call path; if you search for stobj_const_private_ptr in jstracer: guardCallee, moreiter, unboxNextValue; 134 uses in the VM.

Comment 3

[Luke Wagner [:luke]]

Oh right, there was also an unboxing cost before fatvals: (v & ~1).

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Oh right, there was also an unboxing cost before fatvals: (v & ~1).

No - the private slot was storing the pointer value directly. The GC knew how to skip such slots. So I do not see what prevents simply storing the pointer as is in the private slot like in fslots[PRIVATE] = reinterpret_cast<double>(ptr);

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> (In reply to comment #3)
> > Oh right, there was also an unboxing cost before fatvals: (v & ~1).
> 
> No - the private slot was storing the pointer value directly. The GC knew how
> to skip such slots. So I do not see what prevents simply storing the pointer as
> is in the private slot like in fslots[PRIVATE] = reinterpret_cast<double>(ptr);

Or like in *(void **)(fslots + JSSLOT_PRIVATE) = ptr;

Comment 6

[Igor Bukanov]

(In reply to comment #0)
> union {
>   void *privatePtr;
>   fslots[JS_INITIAL_NSLOTS];
> };
> 
> and split the JSObject::init() and initSharingEmptyScope() functions to either
> take a void* or the Value for the (non-private) first fslot.  This avoids
> copying a word on object-init for x86.

Right, this should just work

Comment 7

[Brendan Eich [:brendan]]

private and fslots are not disjoint, see JSFunction. This is important to avoid dslots allocations.

This bug can be fixed independently of bug 558451 but I am tired of rebasing. I'll update bug 558451 with a patch queue closer to land-able soon, but if this bug gets patched sooner, I can cope (after a nap). Rebasing is slowing me down but it is the price of carrying a big patch that cannot easily be landed in pieces without regressing perf or inventing too much temporary machinery -- Luke knows this well from fatvals.

/be

Comment 8

[Luke Wagner [:luke]]

(In reply to comment #7)
> private and fslots are not disjoint, see JSFunction.

I know, the union is just to have privatePtr overlay fslots[0] in a way that didn't require a cast.  But maybe its too confusing.  There are lots of other cast-y ways to achieve the same effect.

Comment 9

[Brendan Eich [:brendan]]

bhackett's patch in bug 584917 just makes private a void* member of JSObject -- brute force and a bit of space wasting, but it enables dslots to point at fslots when possible. Although I'm pretty sure at the cost of some more cycles you could either point dslots at &fslots[0], or at &fslots[1] if JSCLASS_HAS_PRIVATE, and then something like a union would help give void* type to the private pointer.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> bhackett's patch in bug 584917 just makes private a void* member of JSObject --
> brute force and a bit of space wasting, but it enables dslots to point at
> fslots when possible. 

That would also allow to eliminate ensureInstanceReservedSlots as those slots can be stored directly in the private field. To restore the space we should support different-sized objects ideally based on some cache feedback. What is the bug number for that?

Comment 11

[Brendan Eich [:brendan]]

Relevant bugs are:

Bug 547327 - Estimate object size for JS objects
Bug 584917 - TM: Make JSObject flexible length

/be

Comment 12

[Luke Wagner [:luke]]

Attachment: patch

This bug gives a reliable .9% speedup on V8 TM-only.  However, its in the noise for JM+TM.  There is this one unfortunate naming side-effect where "private value", as in js::PrivateValue(fp), is void* boxed as a double (hence markable) and JSSLOT_PRIVATE is an unboxed void*.  To minimize the impact of this, I just made separate overloads for JSObject::init() so that way callers don't need to figure out the correct way to cram a void* into a js::Value.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 474282 [details] [diff] [review]
patch

>-js_NewObjectWithClassProto(JSContext *cx, Class *clasp, JSObject *proto,
>+static JS_ALWAYS_INLINE JSObject*
>+NewObjectWithClassProto(JSContext *cx, Class *clasp, JSObject *proto,
>                            const Value &privateSlotValue)

Fix the overflow line so it's not overindented by 3 spaces.

Looks good otherwise -- makes me want the void *priv; dedicated JSObject member bhackett has going in bug 584917 more and more, though.

Also, I added the trailing cx param just for silly locking but that's going away. Oh well, more patching.

/be

Comment 14

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/672b30ace9bf