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> >>> . >>> >>> -- 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/CADBxricD21KpK1wZXUNpyqOb%2BPX9EvNmhb9QKoUb2LrosU9-hA%40mail.gmail.com.
