Last Comment Bug 703721 - (ObjShrink) Move arguments object private data to a reserved slot
: (ObjShrink) Move arguments object private data to a reserved slot
Status: RESOLVED FIXED
:
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]
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 Brian Hackett (:bhackett) 2011-11-18 12:56:39 PST
Created attachment 575535 [details] [diff] [review]
patch

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.

https://hg.mozilla.org/projects/jaegermonkey/rev/fbfab0e75a63
Comment 2 Brian Hackett (:bhackett) 2011-11-18 13:00:08 PST
Oops, wrong link pasted above.

https://hg.mozilla.org/projects/jaegermonkey/rev/290b3a7329c7
Comment 3 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 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 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-30 15:18:18 PST
Comment on attachment 575535 [details] [diff] [review]
patch

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.
> + *   STACK_FRAME_SLOT

Remove all the tracer-related gunk here before you land this.  \o/
Comment 6 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 Brian Hackett (:bhackett) 2012-01-11 15:06:32 PST
Yes.

https://hg.mozilla.org/mozilla-central/rev/290b3a7329c7

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