Bug 237355 - [WinCairo] Improve WCTiledBacking and TextureMapperSparseBackingStore
Summary: [WinCairo] Improve WCTiledBacking and TextureMapperSparseBackingStore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-01 23:23 PST by Fujii Hironori
Modified: 2022-03-14 14:03 PDT (History)
11 users (show)

See Also:


Attachments
Patch (35.46 KB, patch)
2022-03-01 23:28 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (31.75 KB, patch)
2022-03-02 21:46 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2022-03-01 23:23:50 PST
<https://vscode.dev/> has a very large layer.
WinCairo performance is pretty bad.
Comment 1 Fujii Hironori 2022-03-01 23:28:18 PST
Created attachment 453576 [details]
Patch
Comment 2 Don Olmstead 2022-03-02 09:47:41 PST
Comment on attachment 453576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453576&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:108
> +    class Tile : public BasicRawSentinelNode<Tile> {

Is there a better name for this or the other Tile class? Seems pretty likely to be mixed up.
Comment 3 Fujii Hironori 2022-03-02 12:11:09 PST
Comment on attachment 453576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453576&action=review

>> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:108
>> +    class Tile : public BasicRawSentinelNode<Tile> {
> 
> Is there a better name for this or the other Tile class? Seems pretty likely to be mixed up.

I have no idea. What's your preference? Because they are inter classes, they are distinguishable by using parent class names, WCTileGrid::Tile and TextureMapperSparseBackingStore::Tile.
Comment 4 Don Olmstead 2022-03-02 13:15:34 PST
Comment on attachment 453576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453576&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:60
> +    class TileIndex : public WebCore::IntPoint {
> +    public:
> +        TileIndex() { }
> +        TileIndex(int x, int y)
> +            : WebCore::IntPoint(x, y)
> +        {
> +        }
> +
> +        bool operator<(const TileIndex& other) const
> +        {
> +            if (y() == other.y())
> +                return x() < other.x();
> +            return y() < other.y();
> +        }
> +
> +        bool operator>(const TileIndex& other) const
> +        {
> +            return other < *this;
> +        }
> +

If you're just doing a subclass to tack on operators for `<` and `>` seems like maybe doing a type alias `using TileIndex = WebCore::IntPoint` and adding some functions to do your `<` `>` comparisons might be a better approach.

>>> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:108
>>> +    class Tile : public BasicRawSentinelNode<Tile> {
>> 
>> Is there a better name for this or the other Tile class? Seems pretty likely to be mixed up.
> 
> I have no idea. What's your preference? Because they are inter classes, they are distinguishable by using parent class names, WCTileGrid::Tile and TextureMapperSparseBackingStore::Tile.

I didn't really have anything better.
Comment 5 Darin Adler 2022-03-02 13:27:20 PST
Comment on attachment 453576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453576&action=review

>> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:60
>> +
> 
> If you're just doing a subclass to tack on operators for `<` and `>` seems like maybe doing a type alias `using TileIndex = WebCore::IntPoint` and adding some functions to do your `<` `>` comparisons might be a better approach.

I agree that using custom comparison functions might be better than a custom type.

An alternative is to go further in the other direction, view TileIndex as a true abstraction, in which case we might use IntPoint as the type of a data member rather than deriving from it.

It can help to think about IntPoint the way you would a scalar type like int. If TileIndex contained an int we would not need to derive from int. And we wouldn’t necessarily use strong typing just to define an ordering of int.

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:91
> +        void updateContents(const TileIndex&, Image&, const IntRect& dirtyRect);
> +        void removeTile(const TileIndex&);

For a thing that’s so small and entirely full of scalars, it’s probably more efficient to just use TileIndex rather than const TileIndex&.
Comment 6 Fujii Hironori 2022-03-02 13:29:46 PST
Comment on attachment 453576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453576&action=review

Thank you very much for the review.

>> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:60
>> +
> 
> If you're just doing a subclass to tack on operators for `<` and `>` seems like maybe doing a type alias `using TileIndex = WebCore::IntPoint` and adding some functions to do your `<` `>` comparisons might be a better approach.

Right. I added the comparison operators because I'd like to keep the Tile linked list sorted in ascending order.
However, I don't agree the idea defining the comparison operators for the common IntPoint.
It's not obvious how to define comparison operators for IntPoint, and it might cause surprising confusion.
Comment 7 Darin Adler 2022-03-02 13:40:04 PST
Comment on attachment 453576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453576&action=review

>>>> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:60
>>>> +
>>> 
>>> If you're just doing a subclass to tack on operators for `<` and `>` seems like maybe doing a type alias `using TileIndex = WebCore::IntPoint` and adding some functions to do your `<` `>` comparisons might be a better approach.
>> 
>> I agree that using custom comparison functions might be better than a custom type.
>> 
>> An alternative is to go further in the other direction, view TileIndex as a true abstraction, in which case we might use IntPoint as the type of a data member rather than deriving from it.
>> 
>> It can help to think about IntPoint the way you would a scalar type like int. If TileIndex contained an int we would not need to derive from int. And we wouldn’t necessarily use strong typing just to define an ordering of int.
> 
> Right. I added the comparison operators because I'd like to keep the Tile linked list sorted in ascending order.
> However, I don't agree the idea defining the comparison operators for the common IntPoint.
> It's not obvious how to define comparison operators for IntPoint, and it might cause surprising confusion.

None of this is mandatory, just coding style advice:

The SentinelLInkedList itself does not do the sorting. Our code that enforces the ordering can call functions, doesn’t need to use ">" and "<" operator. If it was a standard library class template or function template that was providing the ordering we would have to supply a "<" function (pre-C++20). This gives us the opportunity to name the ordering. I think it’s best to have this be an IntPoint, but have the linked list use tile index ordering. Maybe just code in terms of an isTileIndexLessThan function (again, pre-C++20, there are slightly more elegant ways once we have more C++20 support).

The need to write encode and decode functions as well as constructors are clues that this may not be the best approach.

It’s a danger sign that this is using a sorted linked list. That is likely not an efficient data structure if there could be a large number of nodes.

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:64
> +        bool operator==(const TileIndex& other) const
> +        {
> +            return x() == other.x() && y() == other.y();
> +        }

Seems highly unlikely this function is needed, even if you retain this design. Pretty sure this is how == will work without writing any explicit code.

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:70
> +        template<class Encoder>
> +        void encode(Encoder& encoder) const
> +        {
> +            encoder << static_cast<const WebCore::IntPoint&>(*this);
> +        }

Seems to me this should work without this. The encoder would just slice the IntPoint? Again, not sure this pattern should be pursued.

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:76
> +        template <class Decoder>
> +        static WARN_UNUSED_RETURN bool decode(Decoder& decoder, TileIndex& result)
> +        {
> +            return decoder.decode(static_cast<WebCore::IntPoint&>(result));
> +        }

The "bool" style is the deprecated style. We prefer the style that returns a std::optional.
Comment 8 Fujii Hironori 2022-03-02 15:50:14 PST
I completely misunderstood Don's comment, and overlooked Darin's comment.
I prefer linked list to hash map because the number of items is small.
If it's true, however, using hash map can't be a problem. And, no longer need the comparison.
I'm going to use HashMap instead of SentinelLinkedList.
Comment 9 Fujii Hironori 2022-03-02 21:46:40 PST
Created attachment 453695 [details]
Patch
Comment 10 Fujii Hironori 2022-03-02 21:49:28 PST
By re-implementing with HashMap, I learned linked list seems better than HashMap for this case.
However, the cost isn't so important and using HashMap makes the code simple.
Comment 11 Fujii Hironori 2022-03-02 21:54:32 PST
Comment on attachment 453576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453576&action=review

>> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:76
>> +        }
> 
> The "bool" style is the deprecated style. We prefer the style that returns a std::optional.

std::optional style decoder looks redundant, so most decoders use old style.
Comment 12 Fujii Hironori 2022-03-07 14:06:35 PST
Comment on attachment 453695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453695&action=review

> Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h:70
> +

If I rewrite this decoder code with the third decoder style (Bug#236246), the code looks like:

    template <class Decoder>
    static std::optional<WCTileUpdate> decode(Decoder& decoder)
    {
        auto index = decoder.decode<WebCore::TextureMapperSparseBackingStore::TileIndex>();
        if (!index)
            return std::nullopt;
        auto willRemove = decoder.decode<bool>();
        if (!willRemove)
            return std::nullopt;
        if (*willRemove)
            return WCTileUpdate { *WTFMove(index), *WTFMove(willRemove) };
        auto backingStore = decoder.decode<WCBackingStore>();
        if (!backingStore)
            return std::nullopt;
        auto dirtyRect = decoder.decode<WebCore::IntRect>();
        if (!dirtyRect)
            return std::nullopt;
        return WCTileUpdate { *WTFMove(index), *WTFMove(willRemove), *WTFMove(backingStore), *WTFMove(dirtyRect) };
    }

Is this really good? Do you like this style? Should all decoders of WebKit be rewritten with this style?
I don't like this style. It looks too verbose and redundant.
Comment 13 Don Olmstead 2022-03-08 15:13:38 PST
Comment on attachment 453695 [details]
Patch

r=me
Comment 14 EWS 2022-03-09 06:31:28 PST
Committed r291048 (248222@main): <https://commits.webkit.org/248222@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453695 [details].
Comment 15 Radar WebKit Bug Importer 2022-03-09 06:32:16 PST
<rdar://problem/90028388>
Comment 16 Darin Adler 2022-03-14 11:09:09 PDT
(In reply to Fujii Hironori from comment #12)
> If I rewrite this decoder code with the third decoder style

I need to do some work on the decoder style. I want to do two things to make decoders much easier to write: one is to make the decoder get into a failed state so that everything decodes are nullopt, so you don’t have to check after decoding one thing before decoding another, another is to write something that helps check many optionals for null all at once so it's easy to write a return statement, maybe something that constructs an argument list and makes an object of some type, or nullopt if any of the optionals passed to it are nullopt.

This should make the decoders better than any current style, much better than all the separate declarations and returns needed for the bool style.
Comment 17 Fujii Hironori 2022-03-14 13:17:40 PDT
IIUC, the decoder will look like:

    template <class Decoder>
    static std::optional<WCTileUpdate> decode(Decoder& decoder)
    {
        auto index = decoder.decode<WebCore::TextureMapperSparseBackingStore::TileIndex>();
        auto willRemove = decoder.decode<bool>();
        if (*willRemove)
            return constuctOrNullOpt<WCTileUpdate>(WTFMove(index), WTFMove(willRemove));
        auto backingStore = decoder.decode<WCBackingStore>();
        auto dirtyRect = decoder.decode<WebCore::IntRect>();
        return constuctOrNullOpt<WCTileUpdate>(WTFMove(index), WTFMove(willRemove), WTFMove(backingStore), WTFMove(dirtyRect) };
    }

But, I want to write a encoder/decoder for a simple default-constructible type like the following:

    template<class Encoder>
    void encode(Encoder& encoder) const
    {
        encoder << index << willRemove << backingStore << dirtyRect;
    }

    template <class Decoder>
    static decode(Decoder& decoder, WCTileUpdate& result)
    {
        decoder >> result.index >> result.willRemove >> result.backingStore >> result.dirtyRect;
    }

If more complicated, 

    template<class Encoder>
    void encode(Encoder& encoder) const
    {
        encoder << index << willRemove;
        if (!willRemove)
            encoder << backingStore << dirtyRect;
    }

    template <class Decoder>
    static decode(Decoder& decoder, WCTileUpdate& result)
    {
        decoder >> result.index >> result.willRemove;
        if (!decoder.didFail() && !willRemove)
                decoder >> result.backingStore >> result.dirtyRect;
    }

I want to use std::optional only for non-default-constructible types.
Comment 18 Darin Adler 2022-03-14 14:03:48 PDT
The direction you describe is almost exactly what I am looking for, but I want something that doesn’t require a total rewrite once you have something more complicated than a struct; I think we can achieve it.