Questions about WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS

[Xianzhu Wang]

Hi,

I'm migrating blink to use gfx geometry types, and noticed that WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS is applied to blink geometry types. It seems to optimize performance of Vector<Type>.

I have the following questions:

1. For a type defined out of blink, how do we force users to include a header delaring WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS(Type)? I'm thinking of putting it in vector_traits.h, but I would like to know if there are better ways.

2. Do we have data showing the performance difference with and without WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS?

Thanks,

Xianzhu

 

[Kentaro Hara]

The macro matters only for objects stored in HeapVector<T> (i.e. Oilpan). So you don't need to export the macro outside Blink :)

2021年11月3日(水) 8:04 Xianzhu Wang <wangx...@chromium.org>:

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADBxriepZahOVJfzQseAyFfkjUPUgLWovXrKUYH54UGY6K2mEw%40mail.gmail.com.

[Fredrik Söderquist]

On Wed, Nov 3, 2021 at 12:29 AM Kentaro Hara <har...@chromium.org> wrote:

The macro matters only for objects stored in HeapVector<T> (i.e. Oilpan). So you don't need to export the macro outside Blink :)

The VectorTraits<T>::kCan{Copy,Move,Fill,Compare}... fields are used for all Vector<T>s though, no? This particular macro doesn't appear to explicitly define/override any Oilpan-related fields AFAICS.

/fs

 

2021年11月3日(水) 8:04 Xianzhu Wang <wangx...@chromium.org>:

Hi,

I'm migrating blink to use gfx geometry types, and noticed that WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS is applied to blink geometry types. It seems to optimize performance of Vector<Type>.

I have the following questions:

1. For a type defined out of blink, how do we force users to include a header delaring WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS(Type)? I'm thinking of putting it in vector_traits.h, but I would like to know if there are better ways.

2. Do we have data showing the performance difference with and without WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS?

Thanks,

Xianzhu

 

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADBxriepZahOVJfzQseAyFfkjUPUgLWovXrKUYH54UGY6K2mEw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jxn-cipEGf%2BU04WVa3UNqLmqmFcX55dWCPy_KK6huAe3A%40mail.gmail.com.

[Kentaro Hara]

The VectorTraits<T>::kCan{Copy,Move,Fill,Compare}... fields are used for all Vector<T>s though, no? This particular macro doesn't appear to explicitly define/override any Oilpan-related fields AFAICS.

You're right -- thanks for pointing it out :)

a) The macros work as a performance optimization for all Vector<T>s. I have no data about the performance numbers though.

b) Some of the macros are mandatory to make HeapVector<T> work correctly (e.g., see static_assert here).

b) does not apply to the geometry types in question. Regarding a), maybe can we just measure performance using benchmarks and see if dropping the macros causes performance regressions? (I hope not.)

--

Kentaro Hara, Tokyo

[Daniel Bratell]

(see inline)

On 2021-11-03 12:15, Kentaro Hara wrote:

The VectorTraits<T>::kCan{Copy,Move,Fill,Compare}... fields are used for all Vector<T>s though, no? This particular macro doesn't appear to explicitly define/override any Oilpan-related fields AFAICS.

You're right -- thanks for pointing it out :)

a) The macros work as a performance optimization for all Vector<T>s. I have no data about the performance numbers though.

The traits have a big effect on micro-benchmarks but it's always hard to map that to user observable performance.

Furthermore, is it possible to replace the macros with use of standard C++ traits such as std::is_trivial? It would require auditing current types, and possibly modifying them, but that is something that has to be done anyway before a complete type merge between Blink and

/Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jxmhADNpdOCAnq_VbH-R%2BVbhJc%3DvS2CAW4LQv10OukU6w%40mail.gmail.com.

[Xianzhu Wang]

One data point: the CL that replaces blink::IntPoint (declared with the macro) with gfx::Point was landed 2 days ago, and I haven't received any performance bug yet. Perhaps we don't use Vector<IntPoint> in performance critical code.

Will try std::is_trivial before converting other types.

[Xianzhu Wang]

Actually we have only two remaining usages of such macros in blink geometry types:

1. float_size.h:

  // Allows this class to be stored in a HeapVector.
  WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS(blink::FloatSize)

  This seems unnecessary because FloatSize is not a garbage collected type, and we don't have any usage of Vector<FloatSize> in blink.

2. int_rect.h:

  WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS(blink::IntRect)

  We have a lot of Vector<IntRect> usages. Will try perf tests.

About std::is_trivial, actually we don't allow particular trivial types to use such macros. For example, in WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS, we have:

   static_assert(!std::is_trivially_default_constructible<ClassName>::value || \

!std::is_trivially_move_assignable<ClassName>::value, \

"macro not needed"); \

Also most of our classes (including all geometry classes) are not trivially default constructible because they do need initialization. We could check if a default constructed object is all zero to see if it can be initialized by memset, but I'm not sure if this is possible at compile time.

It seems that we can check std::is_trivially_copyable and/or std::is_trivially_move_assignable for kCan*WithMemcpy and kCanCompareWithMemcmp.

[Xianzhu Wang]

On Thu, Nov 4, 2021 at 11:38 AM Xianzhu Wang <wangx...@chromium.org> wrote:

Actually we have only two remaining usages of such macros in blink geometry types:

1. float_size.h:

  // Allows this class to be stored in a HeapVector.
  WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS(blink::FloatSize)

  This seems unnecessary because FloatSize is not a garbage collected type, and we don't have any usage of Vector<FloatSize> in blink.

2. int_rect.h:

  WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS(blink::IntRect)

  We have a lot of Vector<IntRect> usages. Will try perf tests.

Perf tests didn't show an obvious difference with WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS(blink::IntRect) removed. The results actually show progressions of total rendering CPU usage, but they are likely to be just noise. See the pinpoint results in https://chromium-review.googlesource.com/c/chromium/src/+/3262556 for details.

So I'm going forward to migrate blink geometry types to gfx types without worrying about WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS on geometry types.

About std::is_trivial, actually we don't allow particular trivial types to use such macros. For example, in WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS, we have:

   static_assert(!std::is_trivially_default_constructible<ClassName>::value || \

!std::is_trivially_move_assignable<ClassName>::value, \

"macro not needed"); \

Also most of our classes (including all geometry classes) are not trivially default constructible because they do need initialization. We could check if a default constructed object is all zero to see if it can be initialized by memset, but I'm not sure if this is possible at compile time.

It seems that we can check std::is_trivially_copyable and/or std::is_trivially_move_assignable for kCan*WithMemcpy and kCanCompareWithMemcmp.

Actually we are already doing this :)

[Kentaro Hara]

+1 to migrate to the gfx types without the macro. Thanks for confirming the performance numbers!

--

Kentaro Hara, Tokyo