Bug 164200 – ClonedArguments need to also support haveABadTime mode.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming.

[Mark Lam]

<rdar://problem/27211336>

[Mark Lam]

Created attachment 293338 [details]
proposed patch.

Let's try this on the EWS bots.  I still need to make sure the DFG and FTL are not making assumptions about the structure of ClonedArguments never mutating (as was hinted by the assertion I changed in the patch).

[Mark Lam]

Comment on attachment 293338 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=293338&action=review

> JSTests/stress/have-a-bad-time-with-direct-arguments-after-warmpup-in-baseline.js:4
> +// If all goes well, this test module will terminate silently. If not, it will crash.
> +// See binary-op-test.js for debugging options if needed.

These are cut-and-paste-o's.  Will fix.

> Source/JavaScriptCore/runtime/ClonedArguments.cpp:126
> -    ASSERT(myFrame->lexicalGlobalObject()->clonedArgumentsStructure() == result->structure());
> +    ASSERT(result->structure(vm)->transitivelyTransitionedFrom(myFrame->lexicalGlobalObject()->clonedArgumentsStructure()));

Hmmm, it does look like the DFG has expectations about the structure of ClonedArguments (see https://trac.webkit.org/changeset/198154).  Intuitively, I would think it is safe to transition structures on the ClonedArguments object.  This is because any JS code should be able to do this, and haveABadTime() does this to the object anyway.  I dig deeper into the DFG code that works with ClonedArguments.

[Mark Lam]

Comment on attachment 293338 [details]
proposed patch.

The fix requires more work.

[Mark Lam]

Created attachment 293800 [details]
proposed patch.

[Mark Lam]

Created attachment 293802 [details]
proposed patch.

[Geoffrey Garen]

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        trivially do indexed property accesses without consulting the Object.prototype.
> +        This defeats many JIT optimizations, and hence, makes the VM "have a bad time".

It's indexed property initialization only that suffers from this problem -- not indexed property loads nor indexed property modification.

> Source/JavaScriptCore/runtime/ClonedArguments.cpp:67
> +        void* temp = vm.heap.tryAllocateAuxiliary(nullptr, Butterfly::totalSize(0, structure->outOfLineCapacity(), true, ArrayStorage::sizeFor(vectorLength)));
> +        if (!temp)
> +            return 0;
> +
> +        butterfly = Butterfly::fromBase(temp, 0, structure->outOfLineCapacity());
> +        ArrayStorage* newStorage = butterfly->arrayStorage();
> +        newStorage->setVectorLength(vectorLength);
> +        newStorage->setLength(length);
> +        newStorage->m_sparseMap.clear();
> +        newStorage->m_indexBias = 0;
> +        newStorage->m_numValuesInVector = vectorLength;
> +
> +        for (unsigned i = 0; i < vectorLength; i++)
> +            newStorage->m_vector[i].clear();
> +

Can we use a helper function here, like JSObject::createArrayStorage?

[Mark Lam]

(In reply to comment #7)
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        trivially do indexed property accesses without consulting the Object.prototype.
> > +        This defeats many JIT optimizations, and hence, makes the VM "have a bad time".
> 
> It's indexed property initialization only that suffers from this problem --
> not indexed property loads nor indexed property modification.

Is this true?  We'll have a bad time if we do any of the following:
   Object.defineProperty(Object.prototype, 0, { get() { return 20 }})
   Object.defineProperty(Object.prototype, 0, { set(x) {} })
   Object.defineProperty(Object.prototype, 0, { })
   Object.defineProperty(Object.prototype, 0, { value: 5 })

Is it not true that HaveABadTime also makes indexed gets slower because every get must now go down the slow path?

> > Source/JavaScriptCore/runtime/ClonedArguments.cpp:67
> > +        void* temp = vm.heap.tryAllocateAuxiliary(nullptr, Butterfly::totalSize(0, structure->outOfLineCapacity(), true, ArrayStorage::sizeFor(vectorLength)));
> > +        if (!temp)
> > +            return 0;
> > +
> > +        butterfly = Butterfly::fromBase(temp, 0, structure->outOfLineCapacity());
> > +        ArrayStorage* newStorage = butterfly->arrayStorage();
> > +        newStorage->setVectorLength(vectorLength);
> > +        newStorage->setLength(length);
> > +        newStorage->m_sparseMap.clear();
> > +        newStorage->m_indexBias = 0;
> > +        newStorage->m_numValuesInVector = vectorLength;
> > +
> > +        for (unsigned i = 0; i < vectorLength; i++)
> > +            newStorage->m_vector[i].clear();
> > +
> 
> Can we use a helper function here, like JSObject::createArrayStorage?

Good point.  I considered this last night, and opted to write the code locally because the pre-existing code for contiguous storage was using a vm.heap.tryAllocateAuxiliary() whereas the JSObject helper functions use vm.heap.allocateAuxiliary().  But looking a little more, I see that the ClonedArguments code has no mode that expects the allocation to ever fail.  I'll look into using the JSObject helpers instead.

[Mark Lam]

(In reply to comment #8)
> (In reply to comment #7)
> > r=me
> > 
> > > Source/JavaScriptCore/ChangeLog:12
> > > +        trivially do indexed property accesses without consulting the Object.prototype.
> > > +        This defeats many JIT optimizations, and hence, makes the VM "have a bad time".
> > 
> > It's indexed property initialization only that suffers from this problem --
> > not indexed property loads nor indexed property modification.
> 
> Is this true?  We'll have a bad time if we do any of the following:
>    Object.defineProperty(Object.prototype, 0, { get() { return 20 }})
>    Object.defineProperty(Object.prototype, 0, { set(x) {} })
>    Object.defineProperty(Object.prototype, 0, { })
>    Object.defineProperty(Object.prototype, 0, { value: 5 })
> 
> Is it not true that HaveABadTime also makes indexed gets slower because
> every get must now go down the slow path?

I spoke with Geoff offline.  haveABadTime only defeats indexed put optimizations.  I will fix the ChangeLog to say this.


> > > Source/JavaScriptCore/runtime/ClonedArguments.cpp:67
> > > +        void* temp = vm.heap.tryAllocateAuxiliary(nullptr, Butterfly::totalSize(0, structure->outOfLineCapacity(), true, ArrayStorage::sizeFor(vectorLength)));
> > > +        if (!temp)
> > > +            return 0;
> > > +
> > > +        butterfly = Butterfly::fromBase(temp, 0, structure->outOfLineCapacity());
> > > +        ArrayStorage* newStorage = butterfly->arrayStorage();
> > > +        newStorage->setVectorLength(vectorLength);
> > > +        newStorage->setLength(length);
> > > +        newStorage->m_sparseMap.clear();
> > > +        newStorage->m_indexBias = 0;
> > > +        newStorage->m_numValuesInVector = vectorLength;
> > > +
> > > +        for (unsigned i = 0; i < vectorLength; i++)
> > > +            newStorage->m_vector[i].clear();
> > > +
> > 
> > Can we use a helper function here, like JSObject::createArrayStorage?
> 
> Good point.  I considered this last night, and opted to write the code
> locally because the pre-existing code for contiguous storage was using a
> vm.heap.tryAllocateAuxiliary() whereas the JSObject helper functions use
> vm.heap.allocateAuxiliary().  But looking a little more, I see that the
> ClonedArguments code has no mode that expects the allocation to ever fail. 
> I'll look into using the JSObject helpers instead.

I'll factor a JSObject::createArrayStorageButterfly() out of JSObject::createArrayStorage(), and use that here instead.

[Mark Lam]

Created attachment 293863 [details]
patch for landing.

[Mark Lam]

Created attachment 293864 [details]
diff between previous patch and the patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r208377: <http://trac.webkit.org/r208377>.