Bug 166907 – Throw an OutOfMemoryError when spread arguments requires too much memory.

• Status: ASSIGNED
• Component: JavaScriptCore

[Mark Lam]

This is friendlier than shutting down with a crash.

[Mark Lam]

<rdar://problem/29867966>

[Mark Lam]

Created attachment 298523 [details]
proposed patch.

Let's try this on the EWS.

[Mark Lam]

Created attachment 298535 [details]
proposed patch.

[Mark Lam]

Created attachment 298549 [details]
proposed patch.

[Mark Lam]

Created attachment 298740 [details]
proposed patch: let's test on those EWS again.

[Mark Lam]

Comment on attachment 298740 [details]
proposed patch: let's test on those EWS again.

This is ready for a review.

[Geoffrey Garen]

Comment on attachment 298740 [details]
proposed patch: let's test on those EWS again.

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

Why is it sufficient to specialize the behavior of JSFixedArray? There are many other ways to allocate large things, and they don't use this mechanism. That makes me think that this fix is either not necessary or not sufficient. 

In this example, we're creating a JSFixedArray that duplicates a JSArray. The JSArray is allowed but the JSFixedArray is prohibited. On its face, this solution is obviously incomplete: A test case that duplicated the JSArray into a JSArray (instead of a JSFixedArray) would certainly still crash.

> Source/JavaScriptCore/heap/AllocationTrait.h:32
> +    MustSucceed, // Allocation will crash if failed.
> +    TryAlloc // Allocation will return nullptr if failed.

It is better to put the comment in the name:

enum class OutOfMemoryBehavior {
    Crash,
    ReturnNull
};

> Source/JavaScriptCore/heap/MarkedSpace.h:283
> +template <AllocationTrait trait>
>  inline void* MarkedSpace::allocateWithDestructor(size_t bytes)
>  {
> +    if (trait == AllocationTrait::TryAlloc)
> +        return tryAllocate(m_destructorSpace, bytes);
>      return allocate(m_destructorSpace, bytes);
>  }

I think it would be clearer to declare separate functions with template specialization.

[Mark Lam]

Comment on attachment 298740 [details]
proposed patch: let's test on those EWS again.

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

>> Source/JavaScriptCore/heap/MarkedSpace.h:283
>>  }
> 
> I think it would be clearer to declare separate functions with template specialization.

The whole point of introducing the AllocationTrait was so that we can write less boiler plate so that we don't have to duplicate every variant of allocate every time we want to use a tryAlloc variant.  I talked to Fil about this got buy in before I started in this.  Making AllocationTrait an enum class does result in the client code being a lot more verbose than I originally intended.  I'll play with some variants and see if I can come up with something that reads better.

[Filip Pizlo]

Comment on attachment 298740 [details]
proposed patch: let's test on those EWS again.

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

>> Source/JavaScriptCore/heap/AllocationTrait.h:32
>> +    TryAlloc // Allocation will return nullptr if failed.
> 
> It is better to put the comment in the name:
> 
> enum class OutOfMemoryBehavior {
>     Crash,
>     ReturnNull
> };

Agreed.  AllocationTrait was confusing to me.

Maybe we should accept OOM as a recognized acronym and call this OOMBehavior.

> Source/JavaScriptCore/heap/Heap.h:407
> +    template<typename T, AllocationTrait> friend void* allocateCell(Heap&);
> +    template<typename T, AllocationTrait> friend void* allocateCell(Heap&, size_t);
> +    template<typename T, AllocationTrait> friend void* allocateCell(Heap&, GCDeferralContext*);
> +    template<typename T, AllocationTrait> friend void* allocateCell(Heap&, GCDeferralContext*, size_t);
> +
> +    template<AllocationTrait = AllocationTrait::MustSucceed> void* allocateWithDestructor(size_t); // For use with objects with destructors.
> +    template<AllocationTrait = AllocationTrait::MustSucceed> void* allocateWithoutDestructor(size_t); // For use with objects without destructors.
> +    template<AllocationTrait = AllocationTrait::MustSucceed> void* allocateWithDestructor(GCDeferralContext*, size_t);
> +    template<AllocationTrait = AllocationTrait::MustSucceed> void* allocateWithoutDestructor(GCDeferralContext*, size_t);
> +    template<typename ClassType, AllocationTrait = AllocationTrait::MustSucceed> void* allocateObjectOfType(size_t); // Chooses one of the methods above based on type.
> +    template<typename ClassType, AllocationTrait = AllocationTrait::MustSucceed> void* allocateObjectOfType(GCDeferralContext*, size_t);

From a user's standpoint, functions called tryAllocate vs allocate are a lot simpler to use than this.  Just a thought.