Bug 237355 – [WinCairo] Improve WCTiledBacking and TextureMapperSparseBackingStore

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

<https://vscode.dev/> has a very large layer.
WinCairo performance is pretty bad.

[Fujii Hironori]

Created attachment 453576 [details]
Patch

[Don Olmstead]

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.

[Fujii Hironori]

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.

[Don Olmstead]

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.

[Darin Adler]

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

[Fujii Hironori]

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.

[Darin Adler]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 453695 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Don Olmstead]

Comment on attachment 453695 [details]
Patch

r=me

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/90028388>

[Darin Adler]

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

[Fujii Hironori]

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.

[Darin Adler]

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.