Changeset 275587 in webkit


Ignore:
Timestamp:
Apr 6, 2021 10:56:24 PM (3 years ago)
Author:
mmaxfield@apple.com
Message:

[GPU Process] Simplify DisplayList::Iterator part 5: Tweak the return type of DisplayList::Iterator::operator*()
​https://bugs.webkit.org/show_bug.cgi?id=224148

Reviewed by Wenson Hsieh.

Source/WebCore:

This patch migrates from

struct Value {

Optional<ItemHandle> item;
Optional<FloatRect> extent;
size_t itemSizeInBuffer { 0 };

};
Value operator*() const;

to

struct Value {

ItemHandle item;
Optional<FloatRect> extent;
size_t itemSizeInBuffer { 0 };

};
Optional<Value> operator*() const

There are two reasons for this:

  1. Philosophically, if the item is nullopt, then all the stuff in the Value is also meaningless
  2. Part of the iterator's API contract is that if the item is nullopt, you're not allowed to keep

iterating - doing this will lead to an infinite loop. Promoting the optional makes it more
likely that this API contract is followed in the future.

No new tests because there is no behavior change.

  • platform/graphics/displaylists/DisplayList.cpp:

(WebCore::DisplayList::DisplayList::asText const):
(WebCore::DisplayList::DisplayList::dump const):

  • platform/graphics/displaylists/DisplayList.h:

(WebCore::DisplayList::DisplayList::iterator::operator* const):

  • platform/graphics/displaylists/DisplayListReplayer.cpp:

(WebCore::DisplayList::Replayer::replay):

Tools:

  • TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:

(TestWebKitAPI::TEST):

Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r275585 r275587  
     12021-04-06  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        [GPU Process] Simplify DisplayList::Iterator part 5: Tweak the return type of DisplayList::Iterator::operator*()
     4        https://bugs.webkit.org/show_bug.cgi?id=224148
     5
     6        Reviewed by Wenson Hsieh.
     7
     8        This patch migrates from
     9
     10        struct Value {
     11            Optional<ItemHandle> item;
     12            Optional<FloatRect> extent;
     13            size_t itemSizeInBuffer { 0 };
     14        };
     15        Value operator*() const;
     16
     17        to
     18
     19        struct Value {
     20            ItemHandle item;
     21            Optional<FloatRect> extent;
     22            size_t itemSizeInBuffer { 0 };
     23        };
     24        Optional<Value> operator*() const
     25
     26        There are two reasons for this:
     27        1. Philosophically, if the item is nullopt, then all the stuff in the Value is also meaningless
     28        2. Part of the iterator's API contract is that if the item is nullopt, you're not allowed to keep
     29               iterating - doing this will lead to an infinite loop. Promoting the optional makes it more
     30               likely that this API contract is followed in the future.
     31
     32        No new tests because there is no behavior change.
     33
     34        * platform/graphics/displaylists/DisplayList.cpp:
     35        (WebCore::DisplayList::DisplayList::asText const):
     36        (WebCore::DisplayList::DisplayList::dump const):
     37        * platform/graphics/displaylists/DisplayList.h:
     38        (WebCore::DisplayList::DisplayList::iterator::operator* const):
     39        * platform/graphics/displaylists/DisplayListReplayer.cpp:
     40        (WebCore::DisplayList::Replayer::replay):
     41
    1422021-04-06  Zalan Bujtas  <zalan@apple.com>
    243

  • trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp

    r275557 r275587  
    115115{
    116116    TextStream stream(TextStream::LineMode::MultipleLine, TextStream::Formatting::SVGStyleRect);
    117     for (auto [item, extent, itemSizeInBuffer] : *this) {
    118         if (!shouldDumpForFlags(flags, *item))
     117    for (auto displayListItem : *this) {
     118        auto [item, extent, itemSizeInBuffer] = displayListItem.value();
     119        if (!shouldDumpForFlags(flags, item))
    119120            continue;
    120121
    121122        TextStream::GroupScope group(stream);
    122123        stream << item;
    123         if (item->isDrawingItem())
     124        if (item.isDrawingItem())
    124125            stream << " extent " << extent;
    125126    }
    … …  
    132133    ts << "display list";
    133134
    134     for (auto [item, extent, itemSizeInBuffer] : *this) {
     135    for (auto displayListItem : *this) {
     136        auto [item, extent, itemSizeInBuffer] = displayListItem.value();
    135137        TextStream::GroupScope group(ts);
    136138        ts << item;
    137         if (item->isDrawingItem())
     139        if (item.isDrawingItem())
    138140            ts << " extent " << extent;
    139141    }

  • trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h

    r275334 r275587  
    11/*
    2  * Copyright (C) 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2016-2021 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    119119
    120120        struct Value {
    121             Optional<ItemHandle> item;
     121            ItemHandle item;
    122122            Optional<FloatRect> extent;
    123123            size_t itemSizeInBuffer { 0 };
    124124        };
    125125
    126         Value operator*() const
     126        Optional<Value> operator*() const
    127127        {
    128             return {
    129                 m_isValid ? makeOptional(ItemHandle { m_currentBufferForItem }) : WTF::nullopt,
     128            if (!m_isValid)
     129                return WTF::nullopt;
     130            return {{
     131                ItemHandle { m_currentBufferForItem },
    130132                m_currentExtent,
    131133                m_currentItemSizeInBuffer,
    132             };
     134            }};
    133135        }
    134136

  • trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp

    r275459 r275587  
    189189#endif
    190190    ReplayResult result;
    191     for (auto [item, extent, itemSizeInBuffer] : m_displayList) {
     191    for (auto displayListItem : m_displayList) {
     192        if (!displayListItem) {
     193            result.reasonForStopping = StopReplayReason::InvalidItemOrExtent;
     194            break;
     195        }
     196
     197        auto [item, extent, itemSizeInBuffer] = displayListItem.value();
     198
    192199        if (!initialClip.isZero() && extent && !extent->intersects(initialClip)) {
    193200            LOG_WITH_STREAM(DisplayLists, stream << "skipping " << i++ << " " << item);
    … …  
    196203        }
    197204
    198         if (!item) {
    199             result.reasonForStopping = StopReplayReason::InvalidItemOrExtent;
    200             break;
    201         }
    202 
    203205        LOG_WITH_STREAM(DisplayLists, stream << "applying " << i++ << " " << item);
    204206
    205         if (item->is<MetaCommandChangeDestinationImageBuffer>()) {
     207        if (item.is<MetaCommandChangeDestinationImageBuffer>()) {
    206208            result.numberOfBytesRead += itemSizeInBuffer;
    207209            result.reasonForStopping = StopReplayReason::ChangeDestinationImageBuffer;
    208             result.nextDestinationImageBuffer = item->get<MetaCommandChangeDestinationImageBuffer>().identifier();
     210            result.nextDestinationImageBuffer = item.get<MetaCommandChangeDestinationImageBuffer>().identifier();
    209211            break;
    210212        }
    211213
    212         if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(*item); reasonForStopping) {
     214        if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(item); reasonForStopping) {
    213215            result.reasonForStopping = *reasonForStopping;
    214216            result.missingCachedResourceIdentifier = WTFMove(missingCachedResourceIdentifier);
    … …  
    219221
    220222        if (UNLIKELY(trackReplayList)) {
    221             replayList->append(*item);
    222             if (item->isDrawingItem())
     223            replayList->append(item);
     224            if (item.isDrawingItem())
    223225                replayList->addDrawingItemExtent(WTFMove(extent));
    224226        }

  • trunk/Source/WebCore/platform/graphics/displaylists/InMemoryDisplayList.cpp

    r275459 r275587  
    6262{
    6363    auto end = this->end();
    64     for (auto [item, extent, size] : *this) {
     64    for (auto displayListItem : *this) {
     65        auto item = displayListItem->item;
    6566        ASSERT(item);
    6667        if (!item)
    6768            break;
    68         item->destroy();
     69        item.destroy();
    6970    }
    7071}

  • trunk/Tools/ChangeLog

    r275581 r275587  
     12021-04-06  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        [GPU Process] Simplify DisplayList::Iterator part 5: Tweak the return type of DisplayList::Iterator::operator*()
     4        https://bugs.webkit.org/show_bug.cgi?id=224148
     5
     6        Reviewed by Wenson Hsieh.
     7
     8        * TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:
     9        (TestWebKitAPI::TEST):
     10
    1112021-04-06  Jean-Yves Avenard  <jya@apple.com>
    212

  • trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp

    r275459 r275587  
    8080
    8181    bool observedUnexpectedItem = false;
    82     for (auto [handle, extent, size] : list) {
    83         switch (handle->type()) {
     82    for (auto displayListItem : list) {
     83        auto handle = displayListItem->item;
     84        switch (handle.type()) {
    8485        case ItemType::SetStrokeThickness: {
    85             EXPECT_FALSE(handle->isDrawingItem());
    86             EXPECT_TRUE(handle->is<SetStrokeThickness>());
    87             auto& item = handle->get<SetStrokeThickness>();
     86            EXPECT_FALSE(handle.isDrawingItem());
     87            EXPECT_TRUE(handle.is<SetStrokeThickness>());
     88            auto& item = handle.get<SetStrokeThickness>();
    8889            EXPECT_EQ(item.thickness(), 1.5);
    8990            break;
    9091        }
    9192        case ItemType::FillPath: {
    92             EXPECT_TRUE(handle->isDrawingItem());
    93             EXPECT_TRUE(handle->is<FillPath>());
    94             auto& item = handle->get<FillPath>();
     93            EXPECT_TRUE(handle.isDrawingItem());
     94            EXPECT_TRUE(handle.is<FillPath>());
     95            auto& item = handle.get<FillPath>();
    9596            EXPECT_EQ(item.path().platformPath(), path.platformPath());
    9697            break;
    9798        }
    9899        case ItemType::FillRectWithGradient: {
    99             EXPECT_TRUE(handle->isDrawingItem());
    100             EXPECT_TRUE(handle->is<FillRectWithGradient>());
    101             auto& item = handle->get<FillRectWithGradient>();
     100            EXPECT_TRUE(handle.isDrawingItem());
     101            EXPECT_TRUE(handle.is<FillRectWithGradient>());
     102            auto& item = handle.get<FillRectWithGradient>();
    102103            EXPECT_EQ(item.rect(), FloatRect(1., 1., 10., 10.));
    103104            EXPECT_EQ(&item.gradient(), gradient.ptr());
    … …  
    105106        }
    106107        case ItemType::SetInlineFillColor: {
    107             EXPECT_FALSE(handle->isDrawingItem());
    108             EXPECT_TRUE(handle->is<SetInlineFillColor>());
    109             auto& item = handle->get<SetInlineFillColor>();
     108            EXPECT_FALSE(handle.isDrawingItem());
     109            EXPECT_TRUE(handle.is<SetInlineFillColor>());
     110            auto& item = handle.get<SetInlineFillColor>();
    110111            EXPECT_EQ(item.color(), Color::red);
    111112            break;
    … …  
    113114#if ENABLE(INLINE_PATH_DATA)
    114115        case ItemType::StrokeInlinePath: {
    115             EXPECT_TRUE(handle->isDrawingItem());
    116             EXPECT_TRUE(handle->is<StrokeInlinePath>());
    117             auto& item = handle->get<StrokeInlinePath>();
     116            EXPECT_TRUE(handle.isDrawingItem());
     117            EXPECT_TRUE(handle.is<StrokeInlinePath>());
     118            auto& item = handle.get<StrokeInlinePath>();
    118119            const auto path = item.path();
    119120            auto& line = path.inlineData<LineData>();
    … …  
    149150    list.append<FillRectWithColor>(FloatRect { 0, 0, 100, 100 }, Color::black);
    150151
    151     for (auto [handle, extent, size] : list) {
    152         EXPECT_EQ(handle->type(), ItemType::FillRectWithColor);
    153         EXPECT_TRUE(handle->is<FillRectWithColor>());
    154 
    155         auto& item = handle->get<FillRectWithColor>();
     152    for (auto displayListItem : list) {
     153        auto handle = displayListItem->item;
     154        EXPECT_EQ(handle.type(), ItemType::FillRectWithColor);
     155        EXPECT_TRUE(handle.is<FillRectWithColor>());
     156
     157        auto& item = handle.get<FillRectWithColor>();
    156158        EXPECT_EQ(*item.color().tryGetAsSRGBABytes(), Color::black);
    157159        EXPECT_EQ(item.rect(), FloatRect(0, 0, 100, 100));
    … …  
    225227
    226228    Vector<ItemType> itemTypes;
    227     for (auto [handle, extent, size] : shallowCopy)
    228         itemTypes.append(handle->type());
     229    for (auto displayListItem : shallowCopy)
     230        itemTypes.append(displayListItem->item.type());
    229231
    230232    EXPECT_FALSE(shallowCopy.isEmpty());
Note: See TracChangeset for help on using the changeset viewer.