Bug 225579 - [GPU Process] Simplify DisplayList::Iterator part 8: Replace the guts of DisplayList::Iterator without affecting its public API
Summary: [GPU Process] Simplify DisplayList::Iterator part 8: Replace the guts of Disp...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 224314
Blocks: 223560 225582
  Show dependency treegraph
 
Reported: 2021-05-09 03:17 PDT by Myles C. Maxfield
Modified: 2021-05-16 03:18 PDT (History)
3 users (show)

See Also:


Attachments
For review (19.30 KB, patch)
2021-05-09 03:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for review (19.73 KB, patch)
2021-05-09 03:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for EWS (34.98 KB, patch)
2021-05-09 03:49 PDT, Myles C. Maxfield
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for review (19.73 KB, patch)
2021-05-09 03:49 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for EWS (35.03 KB, patch)
2021-05-09 03:55 PDT, Myles C. Maxfield
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for review (19.73 KB, patch)
2021-05-09 03:55 PDT, Myles C. Maxfield
sam: review+
Details | Formatted Diff | Diff
Updated patch for EWS (37.39 KB, patch)
2021-05-09 14:28 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (22.08 KB, patch)
2021-05-09 14:29 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-05-09 03:17:40 PDT
[GPU Process] Simplify DisplayList::Iterator part 8: Replace the guts of DisplayList::Iterator without affecting its public API
Comment 1 Myles C. Maxfield 2021-05-09 03:38:42 PDT
Created attachment 428123 [details]
For review
Comment 2 Myles C. Maxfield 2021-05-09 03:48:02 PDT
Created attachment 428124 [details]
Patch for review
Comment 3 Myles C. Maxfield 2021-05-09 03:49:22 PDT
Created attachment 428125 [details]
Patch for EWS
Comment 4 Myles C. Maxfield 2021-05-09 03:49:39 PDT
Created attachment 428126 [details]
Patch for review
Comment 5 Myles C. Maxfield 2021-05-09 03:55:27 PDT
Created attachment 428127 [details]
Patch for EWS
Comment 6 Myles C. Maxfield 2021-05-09 03:55:54 PDT
Created attachment 428128 [details]
Patch for review
Comment 7 Sam Weinig 2021-05-09 09:30:39 PDT
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 8 Sam Weinig 2021-05-09 10:00:29 PDT
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 9 Myles C. Maxfield 2021-05-09 14:26:56 PDT
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!
Comment 10 Myles C. Maxfield 2021-05-09 14:28:51 PDT
Created attachment 428137 [details]
Updated patch for EWS
Comment 11 Myles C. Maxfield 2021-05-09 14:29:35 PDT
Created attachment 428138 [details]
Patch for committing
Comment 12 Myles C. Maxfield 2021-05-10 02:56:06 PDT
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
Comment 13 Radar WebKit Bug Importer 2021-05-16 03:18:17 PDT
<rdar://problem/78073535>