constructArray variants should take the slow path for subclasses of Array
Created attachment 343072 [details] Patch
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?
Comment on attachment 343072 [details] Patch Can you add tests for this? r=me too.
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.
(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.
(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?
Created attachment 343083 [details] Patch
Created attachment 343087 [details] Patch
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?
(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.
Committed r232977: <https://trac.webkit.org/changeset/232977>
<rdar://problem/41260811>