(ObjShrink) Move arguments object private data to a reserved slot




JavaScript Engine
6 years ago
6 years ago


(Reporter: bhackett, Assigned: bhackett)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



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

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

Assignee: general → bhackett1024
Attachment #575535 - Flags: review?


6 years ago
Blocks: 696353


6 years ago
Attachment #575535 - Flags: review? → review?(jwalden+bmo)

Comment 2

6 years ago
Oops, wrong link pasted above.

Peter, bobby, can we move xpconnect off privates and to reserved slots?
(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 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/
Attachment #575535 - Flags: review?(jwalden+bmo) → review+
Depends on: 716013
> 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

6 years ago

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