| Summary: | [WTF] Add Vector&& move constructor / assignment to FixedVector and RefCountedArray | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, ews-watchlist, rniwa, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Yusuke Suzuki
2021-04-13 01:55:06 PDT
Created attachment 425848 [details]
Patch
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. 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. 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. 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. 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. Created attachment 425853 [details]
Patch
Created attachment 425854 [details]
Patch
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]. 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? 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. 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? 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. |