Bug 224475 – [WTF] Add Vector&& move constructor / assignment to FixedVector and RefCountedArray

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Add Vector&& move constructor / assignment to FixedVector and RefCountedArray

[Yusuke Suzuki]

Created attachment 425848 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 425848 [details]
Patch

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

> Source/WTF/wtf/RefCountedArray.h:131
> +        for (unsigned index = 0; index < vector.size(); ++index)
> +            new (data + index) T(WTFMove(vector[index]));

Why not use VectorTypeOperations so that we can use memcpy when w don't need to call constructor?

> Source/WTF/wtf/RefCountedArray.h:183
> +            for (unsigned index = 0; index < vector.size(); ++index)
> +                new (data + index) T(WTFMove(vector[index]));

Ditto.

> Source/WTF/wtf/RefCountedArray.h:188
> +        if (--Header::fromPayload(oldData)->refCount)
> +            return *this;

We want to leave the ref count at 1 because if we decremented to 0,
it's possible that someone else will try to store this in Ref/RefPtr,
and we can end up calling this destruction code again!
We should probably fix ~RefCountedArray() as well.

[Yusuke Suzuki]

Comment on attachment 425848 [details]
Patch

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

>> Source/WTF/wtf/RefCountedArray.h:131
>> +            new (data + index) T(WTFMove(vector[index]));
> 
> Why not use VectorTypeOperations so that we can use memcpy when w don't need to call constructor?

VectorTypeOperations::move is not designed to be used outside of Vector.
That function relies on that the already-moved buffer will be freed without destroying entries.
So, if we use it here, we will see that the same object is destroyed twice (one in the Vector side, one in this RefCountedArray side).

>> Source/WTF/wtf/RefCountedArray.h:183
>> +                new (data + index) T(WTFMove(vector[index]));
> 
> Ditto.

Ditto. It didn't work.

>> Source/WTF/wtf/RefCountedArray.h:188
>> +            return *this;
> 
> We want to leave the ref count at 1 because if we decremented to 0,
> it's possible that someone else will try to store this in Ref/RefPtr,
> and we can end up calling this destruction code again!
> We should probably fix ~RefCountedArray() as well.

I'll change that.

[Ryosuke Niwa]

Comment on attachment 425848 [details]
Patch

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

>>> Source/WTF/wtf/RefCountedArray.h:131
>>> +            new (data + index) T(WTFMove(vector[index]));
>> 
>> Why not use VectorTypeOperations so that we can use memcpy when w don't need to call constructor?
> 
> VectorTypeOperations::move is not designed to be used outside of Vector.
> That function relies on that the already-moved buffer will be freed without destroying entries.
> So, if we use it here, we will see that the same object is destroyed twice (one in the Vector side, one in this RefCountedArray side).

Ah, ok. I guess it's outside the scope of this patch then
but we should probably come up with a way to use memcpy here.

[Ryosuke Niwa]

Comment on attachment 425848 [details]
Patch

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

> Source/WTF/wtf/RefCountedArray.h:128
> +        m_data = data;
> +        Header::fromPayload(data)->refCount = 1;
> +        Header::fromPayload(data)->length = vector.size();

It's really bad that this class keeps repeating this pattern everywhere!
It should be a helper function on Header although it's totally outside the scope of this patch as well.

[Yusuke Suzuki]

Comment on attachment 425848 [details]
Patch

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

>> Source/WTF/wtf/RefCountedArray.h:128
>> +        Header::fromPayload(data)->length = vector.size();
> 
> It's really bad that this class keeps repeating this pattern everywhere!
> It should be a helper function on Header although it's totally outside the scope of this patch as well.

Added allocateUninitializedData helper function.

[Yusuke Suzuki]

Created attachment 425853 [details]
Patch

[Yusuke Suzuki]

Created attachment 425854 [details]
Patch

[EWS]

Committed r275901 (236466@main): <https://commits.webkit.org/236466@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425854 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76605876>

[Darin Adler]

Comment on attachment 425854 [details]
Patch

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

> Source/WTF/wtf/FixedVector.h:88
> +    template<size_t inlineCapacity, typename OverflowHandler>
> +    explicit FixedVector(Vector<T, inlineCapacity, OverflowHandler>&& other)
> +        : m_storage(WTFMove(other))
> +    { }
> +
> +    template<size_t inlineCapacity, typename OverflowHandler>
> +    FixedVector& operator=(Vector<T, inlineCapacity, OverflowHandler>&& other)
> +    {
> +        m_storage = WTFMove(other);
> +        return *this;
> +    }

Don’t we need some calls to shrink to fit here?

[Yusuke Suzuki]

Comment on attachment 425854 [details]
Patch

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

>> Source/WTF/wtf/FixedVector.h:88
>> +    }
> 
> Don’t we need some calls to shrink to fit here?

FixedVector is fixed sized. We always allocate the just-fit size array. There is no shrink operations.

[Darin Adler]

Comment on attachment 425854 [details]
Patch

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

>>> Source/WTF/wtf/FixedVector.h:88
>>> +    }
>> 
>> Don’t we need some calls to shrink to fit here?
> 
> FixedVector is fixed sized. We always allocate the just-fit size array. There is no shrink operations.

Oh, I see now: The storage is RefCountedArray so it’s really the RefCountedArray implementation I need to look at, and that does correctly take only the items from the Vector, and not inherit its memory block, which is what I was hoping for.

That’s so strange! Why always use reference counts in a FixedVector implementation?

[Yusuke Suzuki]

Comment on attachment 425854 [details]
Patch

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

>>>> Source/WTF/wtf/FixedVector.h:88
>>>> +    }
>>> 
>>> Don’t we need some calls to shrink to fit here?
>> 
>> FixedVector is fixed sized. We always allocate the just-fit size array. There is no shrink operations.
> 
> Oh, I see now: The storage is RefCountedArray so it’s really the RefCountedArray implementation I need to look at, and that does correctly take only the items from the Vector, and not inherit its memory block, which is what I was hoping for.
> 
> That’s so strange! Why always use reference counts in a FixedVector implementation?

Yes since we are using RefCountedArray internally. But it is always 1 or 0, so basically it does not matter.
And in terms of size, it does not matter too since we need to have 8byte offset for alignment and 4byte for length and 4byte for refCount.