+1 to migrate to the gfx types without the macro. Thanks for confirming the performance numbers!
On Fri, Nov 5, 2021 at 10:23 AM Xianzhu Wang <[email protected]> wrote: > On Thu, Nov 4, 2021 at 11:38 AM Xianzhu Wang <[email protected]> > wrote: > >> Actually we have only two remaining usages of such macros in blink >> geometry types: >> 1. float_size.h >> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/geometry/float_size.h;drc=134f570f0aff185007cd8a2757f4188567a7df9f;l=240> >> : >> // 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 >> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/geometry/int_rect.h;drc=134f570f0aff185007cd8a2757f4188567a7df9f;l=272> >> : >> 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 > <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/vector_traits.h;drc=c31166996f0c14c8d88f2ca2fdd1bbfce0f9f6f0;l=51> > :) > > >> >> On Thu, Nov 4, 2021 at 10:39 AM Xianzhu Wang <[email protected]> >> wrote: >> >>> One data point: the CL >>> <https://chromium-review.googlesource.com/c/chromium/src/+/3252774> >>> 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. >>> >>> On Thu, Nov 4, 2021 at 10:19 AM Daniel Bratell <[email protected]> >>> wrote: >>> >>>> (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 >>>> <http://www.cplusplus.com/reference/type_traits/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 >>>> >>>> >>>> b) Some of the macros are mandatory to make HeapVector<T> work >>>> correctly (e.g., see static_assert here >>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/heap/collection_support/heap_vector_backing.h;l=81?q=kCanClearUnusedSlotsWithMemset&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2Fplatform%2F> >>>> ). >>>> >>>> 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.) >>>> >>>> >>>> On Wed, Nov 3, 2021 at 7:19 PM Fredrik Söderquist <[email protected]> wrote: >>>> >>>>> On Wed, Nov 3, 2021 at 12:29 AM Kentaro Hara <[email protected]> >>>>> 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 <[email protected]>: >>>>>> >>>>>>> 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 >>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/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 [email protected]. >>>>>>> To view this discussion on the web visit >>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADBxriepZahOVJfzQseAyFfkjUPUgLWovXrKUYH54UGY6K2mEw%40mail.gmail.com >>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADBxriepZahOVJfzQseAyFfkjUPUgLWovXrKUYH54UGY6K2mEw%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> >>>>>> -- >>>>>> 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 [email protected]. >>>>>> 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 >>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jxn-cipEGf%2BU04WVa3UNqLmqmFcX55dWCPy_KK6huAe3A%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>> . >>>>>> >>>>> >>>> >>>> -- >>>> Kentaro Hara, Tokyo >>>> -- >>>> 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 [email protected]. >>>> 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 >>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jxmhADNpdOCAnq_VbH-R%2BVbhJc%3DvS2CAW4LQv10OukU6w%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>> . >>>> >>>> -- Kentaro Hara, Tokyo -- 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 [email protected]. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jysCHT9O7CPyPvAgRooLMk7ZRzpEa%3D6%2BULNx8cVLo7uOg%40mail.gmail.com.
