The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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).
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
Assignee: general → bhackett1024
Attachment #575535 - Flags: review?
Blocks: 696353
Attachment #575535 - Flags: review? → review?(jwalden+bmo)
Oops, wrong link pasted above.

https://hg.mozilla.org/projects/jaegermonkey/rev/290b3a7329c7
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]
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/
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?
Yes.

https://hg.mozilla.org/mozilla-central/rev/290b3a7329c7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.