Micro-optimize HashMap IPC encoder / decoder.
Created attachment 374511 [details] Patch
Created attachment 374512 [details] Patch
Created attachment 374513 [details] Patch
Created attachment 374515 [details] Patch
<rdar://problem/53351435>
Comment on attachment 374515 [details] Patch r=me
Comment on attachment 374515 [details] Patch Clearing flags on attachment: 374515 Committed r247672: <https://trac.webkit.org/changeset/247672>
All reviewed patches have been landed. Closing bug.
Comment on attachment 374515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374515&action=review > Source/WebKit/ChangeLog:17 > + HashMap::size() returns an 'unsigned int' type. Finally, update the modern > + decoder to WTFMove(hashMap) when returning. Because the function returns an > + Optional<HashMap> and not a HashMap, I do not believe we get return value > + optimization (RVO). Apparently we actually do. This triggered a huge -Wredundant-move warning spam in GCC. The full warnings are huge but the interesting parts look like this: DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:568:64: warning: redundant move in return statement [-Wredundant-move] 568 | #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value) | ^ ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:400:16: note: in expansion of macro ‘WTFMove’ 400 | return WTFMove(hashMap); | ^~~~~~~ DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:568:64: note: remove ‘std::move’ call 568 | #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value) | ^ ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:400:16: note: in expansion of macro ‘WTFMove’ 400 | return WTFMove(hashMap); | ^~~~~~~
Committed r247823: <https://trac.webkit.org/changeset/247823>
(In reply to Michael Catanzaro from comment #10) > Committed r247823: <https://trac.webkit.org/changeset/247823> Per spec: """ in a return statement in a function with a class return type, when the expression is the name of a non-volatile automatic object (other than a function or catch-clause parameter) with the same cv-unqualified type as the function return type, the copy/move operation can be omitted by constructing the automatic object directly into the function’s return value """ The return type if not the same so I would not expect RVO to apply here. Did you actually check your compiler warning is not bogus before reverting?
Maybe you're right about that, but WTF::Optional doesn't have a move constructor (it's explicitly deleted), so I think it's still going to copy despite the WTFMove(). If so, I'd say the warning message could certainly stand to be improved.
(In reply to Michael Catanzaro from comment #12) > Maybe you're right about that, but WTF::Optional doesn't have a move > constructor (it's explicitly deleted), so I think it's still going to copy > despite the WTFMove(). If so, I'd say the warning message could certainly > stand to be improved. Huh? Optional(Optional&& rhs) __NOEXCEPT_(detail_::is_nothrow_move_constructible<T>::value) : OptionalBase<T>() { if (rhs.initialized()) { ::new (static_cast<void*>(dataptr())) T(std::move(*rhs)); OptionalBase<T>::init_ = true; rhs.clear(); } } constexpr Optional(const T& v) : OptionalBase<T>(v) {} constexpr Optional(T&& v) : OptionalBase<T>(detail_::constexpr_move(v)) {}
Yeah, the class is confusing. It has a constructor here: Optional(Optional&& rhs) __NOEXCEPT_(detail_::is_nothrow_move_constructible<T>::value) : OptionalBase<T>() { if (rhs.initialized()) { ::new (static_cast<void*>(dataptr())) T(std::move(*rhs)); OptionalBase<T>::init_ = true; rhs.clear(); } } But I think that won't be used here because you don't have an Optional already. But there's a constexpr one below: constexpr Optional(T&& v) : OptionalBase<T>(detail_::constexpr_move(v)) {} Maybe that one gets used? I was looking at: Optional(T&&) = delete; But that's actually a specialization for class Optional<T&>, where T is a reference type... so move constructor is deleted only in that case. Very confusing. Regardless, my experience with suspected compiler bugs is they're few and far between. Usually it's smarter than the humans. Certainly smarter than me. :)
(In reply to Michael Catanzaro from comment #14) > Yeah, the class is confusing. It has a constructor here: > > Optional(Optional&& rhs) > __NOEXCEPT_(detail_::is_nothrow_move_constructible<T>::value) > : OptionalBase<T>() > { > if (rhs.initialized()) { > ::new (static_cast<void*>(dataptr())) T(std::move(*rhs)); > OptionalBase<T>::init_ = true; > rhs.clear(); > } > } > > But I think that won't be used here because you don't have an Optional > already. > > But there's a constexpr one below: > > constexpr Optional(T&& v) : OptionalBase<T>(detail_::constexpr_move(v)) {} > > Maybe that one gets used? > > I was looking at: > > Optional(T&&) = delete; > > But that's actually a specialization for class Optional<T&>, where T is a > reference type... so move constructor is deleted only in that case. Very > confusing. > > Regardless, my experience with suspected compiler bugs is they're few and > far between. Usually it's smarter than the humans. Certainly smarter than > me. :) Ok, I have just validated that the HashMap copy constructor does not get called when IPC decoding after your change. So it looks like it is fine. I am surprised the compiler is able to do RVO here but apparently it can. And yes, it was calling this move constructor: constexpr Optional(T&& v) : OptionalBase<T>(detail_::constexpr_move(v)) {}
Thanks for checking!
BTW the difference between Clang and GCC here is that Clang only has -Wpessimizing-move, where 'return WTFMove(foo)' actually *prevents* RVO from happening. GCC will additionally warn when the move is harmless but not doing anything, -Wredundant-move.