Use an rvalue reference overload of GenericHashTraits::peek() for temporary values
Created attachment 228994 [details] Patch
I don't get why this is a good idea. The getters should return a reference or a copy. By returning a rvalue reference, the call site could modify the value in the hash table.
(In reply to comment #2) > I don't get why this is a good idea. The getters should return a reference or a copy. By returning a rvalue reference, the call site could modify the value in the hash table. The getters return an object of the PeekType type, which is never a reference. GenericHashTraits::peek() should simply pass through the value it receives through the parameter, much like std::forward<>(). Returning an rvalue reference here isn't a problem since it either gets RVO-ed or at worst produces a move constructor in the getter. I'll propose making GenericHashTraits::peek() a template that would deal with universal references, like std::forward<>().
Created attachment 229769 [details] Patch
Comment on attachment 229769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229769&action=review > Source/WTF/wtf/HashTraits.h:70 > + template<typename U> > + static U&& peek(U&& value) { return std::forward<U>(value); } I’d suggest doing this all on one line.
(In reply to comment #3) > (In reply to comment #2) > > I don't get why this is a good idea. The getters should return a reference or a copy. By returning a rvalue reference, the call site could modify the value in the hash table. > > The getters return an object of the PeekType type, which is never a reference. GenericHashTraits::peek() should simply pass through the value it receives through the parameter, much like std::forward<>(). Returning an rvalue reference here isn't a problem since it either gets RVO-ed or at worst produces a move constructor in the getter. I still don't get how that would be correct. If the compiler generates a move constructor in the getter, the real value can be invalidated. That would not only break the hash map, but create security vulnerabilities.
(In reply to comment #6) > (In reply to comment #3) > > (In reply to comment #2) > > > I don't get why this is a good idea. The getters should return a reference or a copy. By returning a rvalue reference, the call site could modify the value in the hash table. > > > > The getters return an object of the PeekType type, which is never a reference. GenericHashTraits::peek() should simply pass through the value it receives through the parameter, much like std::forward<>(). Returning an rvalue reference here isn't a problem since it either gets RVO-ed or at worst produces a move constructor in the getter. > > I still don't get how that would be correct. > If the compiler generates a move constructor in the getter, the real value can be invalidated. That would not only break the hash map, but create security vulnerabilities. I don't think peek ever causes the move constructor to be invoked.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > I don't get why this is a good idea. The getters should return a reference or a copy. By returning a rvalue reference, the call site could modify the value in the hash table. > > > > > > The getters return an object of the PeekType type, which is never a reference. GenericHashTraits::peek() should simply pass through the value it receives through the parameter, much like std::forward<>(). Returning an rvalue reference here isn't a problem since it either gets RVO-ed or at worst produces a move constructor in the getter. > > > > I still don't get how that would be correct. > > If the compiler generates a move constructor in the getter, the real value can be invalidated. That would not only break the hash map, but create security vulnerabilities. > > I don't think peek ever causes the move constructor to be invoked. There is probably something I misunderstand about rvalue references. My understanding is rvalue can be consumed through move semantic, or collapsed to a reference. In this case, if they were to be used with move, that would be semantically incorrect. I don't understand when this change would ever be correct. What am I missing?
Comment on attachment 229769 [details] Patch I discussed this with Darin a bit. This should have an API test that shows we avoid an extra copy with this patch.
Ben suspects, and I think I agree, that there are no extra temporary copies caused by the existing code. We should only fix this problem if it is, indeed, a problem. I suggest adding API tests that demonstrate the copying.
To recap, the extra copy I'm trying remove here is the one performed when passing MappedTraits::emptyValue() through MappedTraits::peek() when there's no entry for the specific key. Existing entries have their value copied when it's being returned, which is correct. The newly-constructed empty values that are being returned should simply be passed through MappedTraits::peek() and RVO-ed, without copies.
Created attachment 230513 [details] Patch
Comment on attachment 230513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230513&action=review I think this patch is fine, but the test doesn’t yet compile on Mac. > Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:162 > + EXPECT_EQ(CopyMoveCounter::constructionCount, 0); > + EXPECT_EQ(CopyMoveCounter::copyCount, 1); > + EXPECT_EQ(CopyMoveCounter::moveCount, 0); Two problems here. First, the expected value is supposed to come first, and the thing we are testing is supposed to come second. Second, the type needs to match, so these need to be 0U and 1U rather than 0 and 1.
Created attachment 237029 [details] Patch Addresses Mac build failures, EXPECT_EQ() argument orders.
Comment on attachment 237029 [details] Patch Clearing flags on attachment: 237029 Committed r172895: <http://trac.webkit.org/changeset/172895>
All reviewed patches have been landed. Closing bug.