[GPU Process] Simplify DisplayList::Iterator part 8: Replace the guts of DisplayList::Iterator without affecting its public API
Created attachment 428123 [details] For review
Created attachment 428124 [details] Patch for review
Created attachment 428125 [details] Patch for EWS
Created attachment 428126 [details] Patch for review
Created attachment 428127 [details] Patch for EWS
Created attachment 428128 [details] Patch for review
Comment on attachment 428128 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=428128&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2363 > +template<typename Item> > +static inline typename std::enable_if_t<!HasIsValid<Item>, bool> isValid(const Item&) I would put the template<> on the same line as the function signature. I think you don't want the static or inline here either. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2369 > +template<typename Item> > +static inline typename std::enable_if_t<HasIsValid<Item>, bool> isValid(const Item& item) I would put the template<> on the same line as the function signature. I think you don't want the static or inline here either. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2379 > +template <typename T> > +constexpr size_t paddedSizeOfTypeAndItemInBytes() I would put the template<> on the same line as the function signature. Alternatively, this might be better as constexpr variable. template<typename T> inline constexpr paddedSizeOfTypeAndItemInBytes = sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), sizeof(T)); Then just used as paddedSizeOfTypeAndItemInBytes<T>.
Comment on attachment 428128 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=428128&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:114 > +inline auto DisplayList::Iterator::currentItem() const -> Optional<CurrentItemResult> This inline is unexpected. What is the intent of it? > Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:116 > + using Sequence = brigand::make_sequence<brigand::ptrdiff_t<0>, WTF::variant_size<DisplayListItem>::value>; I think adding a helper type or renaming Sequence, would make it more clear what this is attempting to do. I believe what this is doing is making an integer sequence from 0 to WTF::variant_size<DisplayListItem>, but what you really want here is a type list of the all the types in the DisplayListItem Variant. I think we can do this more directly, and in a way that will be more useful for other uses of iterating all the types here, by just using a type list directly. I am not entirely sure, but I think you might be able to just use DisplayListItem directly in for_each, since it has a type list as part of its type. e.g. brigand::for_each<DisplayListItem>([&](auto&& type) { using DisplayListItemType = decltype(type); ... Let me know if that works. If it doesn't, there are a few other ways we can convert the Variant<...> into a working brigand::list<...>. > Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:119 > + Optional<CurrentItemResult> result; > + brigand::for_each<Sequence>([&](auto&& type) { At some point I want to make a version of this that allows returning a value. > Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:125 > + if (m_cursor + 1 > endOfCurrentBuffer) In general, I think this code could use a few more comments and/or more explicit named constants explaining what the specific lines are doing. Parsing is hard, so explaining why a + 1, or sizeof(uint64_t) is there is going to help future readers.
Comment on attachment 428128 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=428128&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2379 >> +constexpr size_t paddedSizeOfTypeAndItemInBytes() > > I would put the template<> on the same line as the function signature. > > Alternatively, this might be better as constexpr variable. > > template<typename T> inline constexpr paddedSizeOfTypeAndItemInBytes = sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), sizeof(T)); > > Then just used as paddedSizeOfTypeAndItemInBytes<T>. This almost works, but needs two tweaks: 1. You can't have two symbols with the same name where one is a function and the other is a variable. So I renamed this one. 2. roundUpToMultipleOf(alignof(uint64_t), sizeof(T)) needs to become roundUpToMultipleOf<alignof(uint64_t)>(sizeof(T)) >> Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:114 >> +inline auto DisplayList::Iterator::currentItem() const -> Optional<CurrentItemResult> > > This inline is unexpected. What is the intent of it? Right, this is left over from a previous version of the patch that didn't use brigand. I'll remove it. >> Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:116 >> + using Sequence = brigand::make_sequence<brigand::ptrdiff_t<0>, WTF::variant_size<DisplayListItem>::value>; > > I think adding a helper type or renaming Sequence, would make it more clear what this is attempting to do. > > I believe what this is doing is making an integer sequence from 0 to WTF::variant_size<DisplayListItem>, but what you really want here is a type list of the all the types in the DisplayListItem Variant. > > I think we can do this more directly, and in a way that will be more useful for other uses of iterating all the types here, by just using a type list directly. I am not entirely sure, but I think you might be able to just use DisplayListItem directly in for_each, since it has a type list as part of its type. e.g. > > brigand::for_each<DisplayListItem>([&](auto&& type) { > using DisplayListItemType = decltype(type); > > ... > > > Let me know if that works. If it doesn't, there are a few other ways we can convert the Variant<...> into a working brigand::list<...>. It works! So cool! The "using DisplayListItemType" line needs to be "using DisplayListItemType = typename WTF::RemoveCVAndReference<decltype(type)>::type::type;" though. >> Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:125 >> + if (m_cursor + 1 > endOfCurrentBuffer) > > In general, I think this code could use a few more comments and/or more explicit named constants explaining what the specific lines are doing. Parsing is hard, so explaining why a + 1, or sizeof(uint64_t) is there is going to help future readers. Good idea. I've added some comments, and even some ascii-art!
Created attachment 428137 [details] Updated patch for EWS
Created attachment 428138 [details] Patch for committing
Comment on attachment 428138 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=428138&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Whoops
<rdar://problem/78073535>