Bug 225579 – [GPU Process] Simplify DisplayList::Iterator part 8: Replace the guts of DisplayList::Iterator without affecting its public API

• Component: New Bugs
• Status: NEW

[Myles C. Maxfield]

[GPU Process] Simplify DisplayList::Iterator part 8: Replace the guts of DisplayList::Iterator without affecting its public API

[Myles C. Maxfield]

Created attachment 428123 [details]
For review

[Myles C. Maxfield]

Created attachment 428124 [details]
Patch for review

[Myles C. Maxfield]

Created attachment 428125 [details]
Patch for EWS

[Myles C. Maxfield]

Created attachment 428126 [details]
Patch for review

[Myles C. Maxfield]

Created attachment 428127 [details]
Patch for EWS

[Myles C. Maxfield]

Created attachment 428128 [details]
Patch for review

[Sam Weinig]

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

[Sam Weinig]

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.

[Myles C. Maxfield]

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!

[Myles C. Maxfield]

Created attachment 428137 [details]
Updated patch for EWS

[Myles C. Maxfield]

Created attachment 428138 [details]
Patch for committing

[Myles C. Maxfield]

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

[Radar WebKit Bug Importer]

<rdar://problem/78073535>