Bug 199967 – Micro-optimize HashMap & String IPC decoding

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Micro-optimize HashMap IPC encoder / decoder.

[Chris Dumez]

Created attachment 374511 [details]
Patch

[Chris Dumez]

Created attachment 374512 [details]
Patch

[Chris Dumez]

Created attachment 374513 [details]
Patch

[Chris Dumez]

Created attachment 374515 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/53351435>

[Geoffrey Garen]

Comment on attachment 374515 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 374515 [details]
Patch

Clearing flags on attachment: 374515

Committed r247672: <https://trac.webkit.org/changeset/247672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

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);
      |                ^~~~~~~

[Michael Catanzaro]

Committed r247823: <https://trac.webkit.org/changeset/247823>

[Chris Dumez]

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

[Michael Catanzaro]

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.

[Chris Dumez]

(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)) {}

[Michael Catanzaro]

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

[Chris Dumez]

(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)) {}

[Michael Catanzaro]

Thanks for checking!

[Michael Catanzaro]

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.