GenericHashTraits::peek() is producing copies of passed-in temporary values

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

Use an rvalue reference overload of GenericHashTraits::peek() for temporary values

[Zan Dobersek]

Created attachment 228994 [details]
Patch

[Benjamin Poulain]

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.

[Zan Dobersek]

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

[Zan Dobersek]

Created attachment 229769 [details]
Patch

[Darin Adler]

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.

[Benjamin Poulain]

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

[Anders Carlsson]

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

[Benjamin Poulain]

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

[Benjamin Poulain]

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.

[Darin Adler]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 230513 [details]
Patch

[Darin Adler]

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.

[Zan Dobersek]

Created attachment 237029 [details]
Patch

Addresses Mac build failures, EXPECT_EQ() argument orders.

[WebKit Commit Bot]

Comment on attachment 237029 [details]
Patch

Clearing flags on attachment: 237029

Committed r172895: <http://trac.webkit.org/changeset/172895>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.