Bug 234990 – JSArray::fastSlice() should not convert the source from CoW

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

... since we aren't modifying it.

[Alexey Shvayka]

Created attachment 448801 [details]
Patch

[Mark Lam]

Comment on attachment 448801 [details]
Patch

r=me if EWS is happy.

[Yusuke Suzuki]

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.

[Alexey Shvayka]

Created attachment 448806 [details]
Patch

Unset CopyOnWrite from indexing mode.

[Mark Lam]

(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.

[Alexey Shvayka]

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!

[Yusuke Suzuki]

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.

[Alexey Shvayka]

(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?

[Alexey Shvayka]

(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.

[Alexey Shvayka]

Created attachment 448810 [details]
Patch

Assert that result of fastSlice() is non-CoW and leverage indexingType().

[Radar WebKit Bug Importer]

<rdar://problem/87620060>

[Alexey Shvayka]

Comment on attachment 448810 [details]
Patch

Thanks again for review, Yusuke! indexingType() is neat.

[EWS]

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].