Bug 164200 - ClonedArguments need to also support haveABadTime mode.
Summary: ClonedArguments need to also support haveABadTime mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-30 00:17 PDT by Mark Lam
Modified: 2016-11-03 23:22 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (20.76 KB, patch)
2016-10-30 02:00 PDT, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch. (23.36 KB, patch)
2016-11-03 14:00 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (23.36 KB, patch)
2016-11-03 14:28 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff
patch for landing. (25.49 KB, patch)
2016-11-03 22:15 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
diff between previous patch and the patch for landing. (6.60 KB, patch)
2016-11-03 22:16 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-10-30 00:17:45 PDT
Patch coming.
Comment 1 Mark Lam 2016-10-30 00:19:13 PDT
<rdar://problem/27211336>
Comment 2 Mark Lam 2016-10-30 02:00:33 PDT
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).
Comment 3 Mark Lam 2016-10-30 02:21:21 PDT
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.
Comment 4 Mark Lam 2016-11-02 15:28:05 PDT
Comment on attachment 293338 [details]
proposed patch.

The fix requires more work.
Comment 5 Mark Lam 2016-11-03 14:00:03 PDT
Created attachment 293800 [details]
proposed patch.
Comment 6 Mark Lam 2016-11-03 14:28:26 PDT
Created attachment 293802 [details]
proposed patch.
Comment 7 Geoffrey Garen 2016-11-03 14:39:38 PDT
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?
Comment 8 Mark Lam 2016-11-03 15:26:07 PDT
(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.
Comment 9 Mark Lam 2016-11-03 22:07:54 PDT
(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.
Comment 10 Mark Lam 2016-11-03 22:15:29 PDT
Created attachment 293863 [details]
patch for landing.
Comment 11 Mark Lam 2016-11-03 22:16:23 PDT
Created attachment 293864 [details]
diff between previous patch and the patch for landing.
Comment 12 Mark Lam 2016-11-03 23:22:12 PDT
Thanks for the review.  Landed in r208377: <http://trac.webkit.org/r208377>.