[webkit-reviews] review granted: [Bug 186812] constructArray variants should take the slow path for subclasses of Array : [Attachment 343072] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 186812: constructArray variants should take the slow path for subclasses of
Array
https://bugs.webkit.org/show_bug.cgi?id=186812

Attachment 343072: Patch

https://bugs.webkit.org/attachment.cgi?id=343072&action=review




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 343072
  --> https://bugs.webkit.org/attachment.cgi?id=343072
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +	   allocate a new structure for an indexing type change while
initializing

Why are we doing an indexing type change? Why would that only effect sublasses?

> Source/JavaScriptCore/runtime/JSArray.cpp:1411
> +JSArray* constructArray(ExecState* exec, Structure* arrayStructure, const
ArgList& values)
> +{
> +    VM& vm = exec->vm();
> +    unsigned length = values.size();
> +    ObjectInitializationScope scope(vm);
> +
> +    // FIXME: We only need this for subclasses of Array because we might
need to allocate a new structure to change
> +    // indexing types while initializing. If this triggered a GC then we
might scan our currently uninitialized
> +    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
> +    JSArray* array;
> +    if
(arrayStructure->globalObject()->isOriginalArrayStructure(arrayStructure))
> +	   array = JSArray::tryCreateUninitializedRestricted(scope,
arrayStructure, length);
> +    else
> +	   array = JSArray::create(vm, arrayStructure, length);
> +
> +    // FIXME: we should probably throw an out of memory error here, but
> +    // when making this change we should check that all clients of this
> +    // function will correctly handle an exception being thrown from here.
> +    // https://bugs.webkit.org/show_bug.cgi?id=169786
> +    RELEASE_ASSERT(array);
> +
> +    for (unsigned i = 0; i < length; ++i)
> +	   array->initializeIndex(scope, i, values.at(i));
> +    return array;
> +}
> +
> +JSArray* constructArray(ExecState* exec, Structure* arrayStructure, const
JSValue* values, unsigned length)
> +{
> +    VM& vm = exec->vm();
> +    ObjectInitializationScope scope(vm);
> +    // FIXME: We only need this for subclasses of Array because we might
need to allocate a new structure to change
> +    // indexing types while initializing. If this triggered a GC then we
might scan our currently uninitialized
> +    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
> +    JSArray* array;
> +    if
(arrayStructure->globalObject()->isOriginalArrayStructure(arrayStructure))
> +	   array = JSArray::tryCreateUninitializedRestricted(scope,
arrayStructure, length);
> +    else
> +	   array = JSArray::create(vm, arrayStructure, length);
> +
> +    // FIXME: we should probably throw an out of memory error here, but
> +    // when making this change we should check that all clients of this
> +    // function will correctly handle an exception being thrown from here.
> +    // https://bugs.webkit.org/show_bug.cgi?id=169786
> +    RELEASE_ASSERT(array);
> +
> +    for (unsigned i = 0; i < length; ++i)
> +	   array->initializeIndex(scope, i, values[i]);
> +    return array;
> +}
> +
> +JSArray* constructArrayNegativeIndexed(ExecState* exec, Structure*
arrayStructure, const JSValue* values, unsigned length)
> +{
> +    VM& vm = exec->vm();
> +    ObjectInitializationScope scope(vm);
> +    // FIXME: We only need this for subclasses of Array because we might
need to allocate a new structure to change
> +    // indexing types while initializing. If this triggered a GC then we
might scan our currently uninitialized
> +    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
> +    JSArray* array;
> +    if
(arrayStructure->globalObject()->isOriginalArrayStructure(arrayStructure))
> +	   array = JSArray::tryCreateUninitializedRestricted(scope,
arrayStructure, length);
> +    else
> +	   array = JSArray::create(vm, arrayStructure, length);
> +
> +    // FIXME: we should probably throw an out of memory error here, but
> +    // when making this change we should check that all clients of this
> +    // function will correctly handle an exception being thrown from here.
> +    // https://bugs.webkit.org/show_bug.cgi?id=169786
> +    RELEASE_ASSERT(array);
> +
> +    for (int i = 0; i < static_cast<int>(length); ++i)
> +	   array->initializeIndex(scope, i, values[-i]);
> +    return array;
> +}

So much repeated code. Can we abstract this in some way?

Also, is inlining this really not profitable?