Bug 225582

Summary:

[GPU Process] Simplify DisplayList::Iterator part 9: Migrate DisplayList::Iterator's API to use DisplayListItem rather than ItemHandle, and update callers

Product:

WebKit

Reporter:

Myles C. Maxfield <mmaxfield>

Component:

New Bugs

Assignee:

Myles C. Maxfield <mmaxfield>

Status:

ASSIGNED

Severity:

Normal

CC:

sam, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

225579

Bug Blocks:

223560

Attachments:

Description	Flags
WIP	none
Patch for reviewing	sam: review+
Patch for EWS	ews-feeder: commit-queue-
Patch for committing	none
Patch for committing	none
Updated patch for EWS	none

Myles C. Maxfield

Reported 2021-05-09 15:57:36 PDT

Migrate DisplayList::Iterator's API to use DisplayListItem rather than ItemHandle, and update callers

Attachments
WIP (47.20 KB, patch) 2021-05-09 15:59 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch for reviewing (51.72 KB, patch) 2021-05-09 17:37 PDT, Myles C. Maxfield	sam: review+	Details Formatted Diff Diff
Patch for EWS (81.39 KB, patch) 2021-05-09 17:40 PDT, Myles C. Maxfield	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch for committing (55.50 KB, patch) 2021-05-10 02:54 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch for committing (54.86 KB, patch) 2021-05-10 02:58 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Updated patch for EWS (83.83 KB, patch) 2021-05-10 02:59 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Myles C. Maxfield

Comment 1 2021-05-09 15:59:22 PDT

Created attachment 428142 [details] WIP

Myles C. Maxfield

Comment 2 2021-05-09 17:37:39 PDT

Created attachment 428145 [details] Patch for reviewing

Myles C. Maxfield

Comment 3 2021-05-09 17:40:36 PDT

Created attachment 428146 [details] Patch for EWS

Sam Weinig

Comment 4 2021-05-09 18:36:02 PDT

Comment on attachment 428145 [details] Patch for reviewing View in context: https://bugs.webkit.org/attachment.cgi?id=428145&action=review > Source/WebCore/ChangeLog:12 > + - DisplayList::append() goes from 134 lines to 3 Feels good, right? > Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:90 > + auto visitor = WTF::makeVisitor([&](const SetState& stateItem) -> bool { I've been encouraging use of the helper WTF::switchOn(variant, ...) which wraps makeVisitor() and just reads a bit nicer in my opinion. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2385 > +inline constexpr bool isDrawingItem(const DisplayListItem& displayListItem) > +{ > + return WTF::visit([](const auto& displayListItem) { > + return displayListItem.isDrawingItem; > + }, displayListItem); > +} This will end up being a bit of a big function due to all the cases it needs to handle so unless it is particularly performance sensitive, I would recommend moving the implementation out of line. I don't think the constexpr will apply in most cases as you usually don't have a constexpr DisplayListItem. > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:115 > +template<typename Item> > +static inline typename std::enable_if_t<!HasApply<Item>, void> apply(const Item&, GraphicsContext&) These should go on the same line. > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:120 > +template<typename Item> > +static inline typename std::enable_if_t<HasApply<Item>, void> apply(const Item& item, GraphicsContext& graphicsContext) These should go on the same line.

Myles C. Maxfield

Comment 5 2021-05-10 02:54:06 PDT

Created attachment 428159 [details] Patch for committing

Myles C. Maxfield

Comment 6 2021-05-10 02:58:35 PDT

Created attachment 428160 [details] Patch for committing

Myles C. Maxfield

Comment 7 2021-05-10 02:59:09 PDT

Created attachment 428161 [details] Updated patch for EWS

Radar WebKit Bug Importer

Comment 8 2021-05-16 15:58:19 PDT

<rdar://problem/78084300>

Sam Weinig

Comment 9 2021-05-23 08:52:10 PDT

Myles, you still planning on landing these? Would be great to get them in.

Myles C. Maxfield

Comment 10 2021-05-23 10:55:58 PDT

(In reply to Sam Weinig from comment #9) > Myles, you still planning on landing these? Would be great to get them in. I am, but they cause a MotionMark regression. I need to figure out why.

Note You need to log in before you can comment on or make changes to this bug.