Bug 186812 – constructArray variants should take the slow path for subclasses of Array

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

constructArray variants should take the slow path for subclasses of Array

[Keith Miller]

Created attachment 343072 [details]
Patch

[Saam Barati]

Comment on attachment 343072 [details]
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?

[Mark Lam]

Comment on attachment 343072 [details]
Patch

Can you add tests for this?  r=me too.

[Keith Miller]

Comment on attachment 343072 [details]
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?

For non-subclasses we have the structure cached on the global object so no allocation happens. We do an indexing type change because before the allocation profile is set up we don't know what values we are going to stick into the array.

It's possible we should be scanning the arguments to compute the indexing type we should use. This would avoid a late indexing type change and rewriting the butterfly.

>> Source/JavaScriptCore/runtime/JSArray.cpp:1411
>> +}
> 
> So much repeated code. Can we abstract this in some way?
> 
> Also, is inlining this really not profitable?

I guess we could use a template and have a functor that provides the arguments.

[Keith Miller]

(In reply to Mark Lam from comment #3)
> Comment on attachment 343072 [details]
> Patch
> 
> Can you add tests for this?  r=me too.

This is fixing already broken tests.

[Mark Lam]

(In reply to Keith Miller from comment #5)
> (In reply to Mark Lam from comment #3)
> > Comment on attachment 343072 [details]
> > Patch
> > 
> > Can you add tests for this?  r=me too.
> 
> This is fixing already broken tests.

nit: can you note in the ChangeLog that this is covered by existing tests?

[Keith Miller]

Created attachment 343083 [details]
Patch

[Keith Miller]

Created attachment 343087 [details]
Patch

[Saam Barati]

Comment on attachment 343072 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSArray.cpp:1411
>>> +}
>> 
>> So much repeated code. Can we abstract this in some way?
>> 
>> Also, is inlining this really not profitable?
> 
> I guess we could use a template and have a functor that provides the arguments.

What about making this no longer inlined?

[Keith Miller]

(In reply to Saam Barati from comment #9)
> Comment on attachment 343072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343072&action=review
> 
> >>> Source/JavaScriptCore/runtime/JSArray.cpp:1411
> >>> +}
> >> 
> >> So much repeated code. Can we abstract this in some way?
> >> 
> >> Also, is inlining this really not profitable?
> > 
> > I guess we could use a template and have a functor that provides the arguments.
> 
> What about making this no longer inlined?

Oh I doubt that matters since we don't really have information about the structure and the allocation methods were already out of line.

[Keith Miller]

Committed r232977: <https://trac.webkit.org/changeset/232977>

[Radar WebKit Bug Importer]

<rdar://problem/41260811>