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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

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

7 years ago
What reads the privatePtr so much?
(Assignee)

Comment 2

7 years ago
Its on the call path; if you search for stobj_const_private_ptr in jstracer: guardCallee, moreiter, unboxNextValue; 134 uses in the VM.
(Assignee)

Comment 3

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

Comment 4

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

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

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

Comment 8

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

7 years ago
(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?
Relevant bugs are:

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

/be
(Assignee)

Comment 12

7 years ago
Created attachment 474282 [details] [diff] [review]
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.
Assignee: general → lw
Status: NEW → ASSIGNED
Attachment #474282 - Flags: review?(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
Attachment #474282 - Flags: review?(brendan) → review+
(Assignee)

Comment 14

7 years ago
http://hg.mozilla.org/tracemonkey/rev/672b30ace9bf
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.