Last Comment Bug 703721 - (ObjShrink) Move arguments object private data to a reserved slot
: (ObjShrink) Move arguments object private data to a reserved slot
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on: 716013
Blocks: 696353
  Show dependency treegraph
Reported: 2011-11-18 12:49 PST by Brian Hackett (:bhackett)
Modified: 2012-01-11 15:06 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (20.11 KB, patch)
2011-11-18 12:56 PST, Brian Hackett (:bhackett)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description User image Brian Hackett (:bhackett) 2011-11-18 12:49:16 PST
ArgumentsObject VM performance is hurt by the object shrink stuff --- getPrivate/setPrivate are all slower now, along with getSlot/setSlot on the reserved slots (to a lesser degree).  These accesses are all pretty hot in code which manipulates arguments objects and which are not caught by the lazy arguments optimization, which is especially important for the Prototype library as it calls $A(arguments) frequently (which makes a proper array out of the arguments object).
Comment 1 User image Brian Hackett (:bhackett) 2011-11-18 12:56:39 PST
Created attachment 575535 [details] [diff] [review]

Move arguments private data to a reserved slot, use {get,set}FixedSlot for accessing function reserved slots, and enable the has-always-been-disabled arguments IC.
Comment 2 User image Brian Hackett (:bhackett) 2011-11-18 13:00:08 PST
Oops, wrong link pasted above.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2011-11-19 03:05:19 PST
Peter, bobby, can we move xpconnect off privates and to reserved slots?
Comment 4 User image Bobby Holley (:bholley) (busy with Stylo) 2011-11-19 18:50:02 PST
(In reply to Boris Zbarsky (:bz) from comment #3)
> Peter, bobby, can we move xpconnect off privates and to reserved slots?

Well, if nothing else that would change the GC semantics, no?

My impression has always been that there are two differences between privates and reserved slots:
1 - Reserved slots are visible to the GC, privates are not.
2 - You can have 0 or 1 privates, but an arbitrary number of reserved slots.

Are there other differences? If there aren't, it seems like privates are what XPConnect really wants, but maybe I'm missing something...
Comment 5 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-30 15:18:18 PST
Comment on attachment 575535 [details] [diff] [review]

Review of attachment 575535 [details] [diff] [review]:

::: js/src/jsfun.cpp
@@ +140,5 @@
>      bool strict = callee.toFunction()->inStrictMode();
>      Class *clasp = strict ? &StrictArgumentsObjectClass : &NormalArgumentsObjectClass;
>      Shape *emptyArgumentsShape =
>          EmptyShape::lookupInitialShape(cx, clasp, proto,
> +                                       proto->getParent(), FINALIZE_OBJECT4,

Make this gc::FINALIZE_OBJECT4 while you're here.

No, better yet, make static const ArgumentsObject::AllocKind = gc::FINALIZE_OBJECT4, and use that named constant instead of the allockind directly.

@@ +160,1 @@
>      JSObject *obj = js_NewGCObject(cx, FINALIZE_OBJECT4);

Use the same static const again.

@@ -603,5 @@
> - * generator object), we use the JSFRAME_FLOATING_GENERATOR flag, which is only
> - * set on the StackFrame kept in the generator object's JSGenerator.
> - */
> -static inline void
> -MaybeMarkGenerator(JSTracer *trc, JSObject *obj)

MaybeMarkGenerator taking two different kinds of objects was definitely dodgy.  Yet, the repetition of the contents here seems bad.  Change MaybeMarkGenerator to take a StackFrame* fp, passing whichever of the two values makes sense, and keep it, please?

::: js/src/methodjit/PolyIC.cpp
@@ +2525,5 @@
>      JS_ASSERT(hasInlineTypeGuard() || idRemat.knownType() == JSVAL_TYPE_INT32);
>      Assembler masm;
> +    Jump shapeGuard = masm.testObjClass(Assembler::NotEqual, objReg, typeReg, obj->getClass());

Should this be classGuard now?

@@ +2762,5 @@
>          return attachGetProp(f, obj, v, id, vp);
> +    if (obj->isArguments())
> +        return attachArguments(f, obj, v, id, vp);
> +

This should be a separate change from the rest of this patch, not piggybacking on it.  Having mMultiple distinct, separable changes in a single revision makes figuring out what exactly caused a regression harder than it need be.

::: js/src/vm/ArgumentsObject-inl.h
@@ +85,5 @@
>  inline ArgumentsData *
>  ArgumentsObject::data() const
>  {
> +    return reinterpret_cast<js::ArgumentsData *>(getFixedSlot(DATA_SLOT).toPrivate());

Remove the js:: cast while you're touching this.

@@ +111,5 @@
>  inline js::StackFrame *
>  ArgumentsObject::maybeStackFrame() const
>  {
> +    return reinterpret_cast<js::StackFrame *>(getFixedSlot(STACK_FRAME_SLOT).toPrivate());

Remove the js:: cast while you're touching this.

::: js/src/vm/ArgumentsObject.h
@@ +142,5 @@
>   *     element/addressOfElement/setElement to access the values stored in
>   *     the ArgumentsData.  If you're simply looking to get arguments[i],
>   *     however, use getElement or getElements to avoid spreading arguments
>   *     object implementation details around too much.

Remove all the tracer-related gunk here before you land this.  \o/
Comment 6 User image Gary Kwong [:gkw] [:nth10sd] 2012-01-11 15:04:36 PST
> Comment on attachment 575535 [details] [diff] [review]
> patch
> Review of attachment 575535 [details] [diff] [review]:

Did this r+'ed patch make it to m-c?
Comment 7 User image Brian Hackett (:bhackett) 2012-01-11 15:06:32 PST

Note You need to log in before you can comment on or make changes to this bug.