... since we aren't modifying it.
Created attachment 448801 [details] Patch
Comment on attachment 448801 [details] Patch r=me if EWS is happy.
Comment on attachment 448801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448801&action=review > Source/JavaScriptCore/runtime/JSArray.cpp:747 > + Structure* resultStructure = globalObject->arrayStructureForIndexingTypeDuringAllocation(sourceIndexingMode | IsArray); Doesn't it accidentally propagate CoW state if source is CoW? The result of slice should not be CoW array.
Created attachment 448806 [details] Patch Unset CopyOnWrite from indexing mode.
(In reply to Yusuke Suzuki from comment #3) > Comment on attachment 448801 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448801&action=review > > > Source/JavaScriptCore/runtime/JSArray.cpp:747 > > + Structure* resultStructure = globalObject->arrayStructureForIndexingTypeDuringAllocation(sourceIndexingMode | IsArray); > > Doesn't it accidentally propagate CoW state if source is CoW? > The result of slice should not be CoW array. You are right. I actually checked for that during my review, and then got distracted and forgot to follow thru on it. Thanks for catching this.
Comment on attachment 448806 [details] Patch (In reply to Mark Lam from comment #2) > Comment on attachment 448801 [details] > Patch > > r=me if EWS is happy. Thanks Mark! Needs another review I guess. (In reply to Yusuke Suzuki from comment #3) > Comment on attachment 448801 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448801&action=review > > > Source/JavaScriptCore/runtime/JSArray.cpp:747 > > + Structure* resultStructure = globalObject->arrayStructureForIndexingTypeDuringAllocation(sourceIndexingMode | IsArray); > > Doesn't it accidentally propagate CoW state if source is CoW? > The result of slice should not be CoW array. My bad, it does, I didn't look into arrayStructureForIndexingTypeDuringAllocation() close enough. JSTests/microbenchmarks/lazy-array-species-watchpoints.js nicely covers the case. I very much appreciate the tip!
Comment on attachment 448806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448806&action=review r=me with comment. > JSTests/stress/array-slice-cow.js:11 > + return slice; Can we have a test checking slice is non CoW? > JSTests/stress/array-slice-cow.js:20 > + return slice; Ditto > JSTests/stress/array-slice-cow.js:29 > + return slice; Ditto. > Source/JavaScriptCore/runtime/JSArray.cpp:736 > + auto arrayType = (source->indexingMode() & ~CopyOnWrite) | IsArray; We should take indexingType() instead of indexingMode(). The difference between IndexingType and IndexingMode is CoW bit.
(In reply to Yusuke Suzuki from comment #7) > > Source/JavaScriptCore/runtime/JSArray.cpp:736 > > + auto arrayType = (source->indexingMode() & ~CopyOnWrite) | IsArray; > > We should take indexingType() instead of indexingMode(). The difference > between IndexingType and IndexingMode is CoW bit. I'm likely missing something, but in inline IndexingType JSCell::indexingType() const { return indexingTypeAndMisc() & AllWritableArrayTypes; } doesn't `& AllWritableArrayTypes` bit exclude both non-JSArrays and CoW?
(In reply to Alexey Shvayka from comment #8) > (In reply to Yusuke Suzuki from comment #7) > > > Source/JavaScriptCore/runtime/JSArray.cpp:736 > > > + auto arrayType = (source->indexingMode() & ~CopyOnWrite) | IsArray; > > > > We should take indexingType() instead of indexingMode(). The difference > > between IndexingType and IndexingMode is CoW bit. > > I'm likely missing something, but in > > inline IndexingType JSCell::indexingType() const > { > return indexingTypeAndMisc() & AllWritableArrayTypes; > } > > doesn't `& AllWritableArrayTypes` bit exclude both non-JSArrays and CoW? Oh no it doesn't, please disregard.
Created attachment 448810 [details] Patch Assert that result of fastSlice() is non-CoW and leverage indexingType().
<rdar://problem/87620060>
Comment on attachment 448810 [details] Patch Thanks again for review, Yusuke! indexingType() is neat.
Committed r288036 (246062@main): <https://commits.webkit.org/246062@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448810 [details].