1676909 - Too much texture upload in Fenix on https://www.lucidchart.com/pages/de

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Markus Stange [:mstange]]

(Broken out from bug 1673986:)

Scrolling on https://www.lucidchart.com/pages/de in Fenix is very sluggish because of large amounts of texture upload.

Profile: https://share.firefox.dev/2Iyh7cD

Comment 1

[Markus Stange [:mstange]]

On my Moto G5 this page actually crashes the browser, but I don't know where (due to #16529).

Comment 2

[Jamie Nicol [:jnicol]]

Our problem lies here:

.css-10lk9rh::before {
    background: url('https://d2slcw3kip6qmk.cloudfront.net/marketing/pages/chart/Homepage/background.svg');
    background-repeat: no-repeat;
    background-size: cover;
    z-index: -99;
    height: 100%;
    content: "";
    position: absolute;
    left: 0;
    right: 0;
    top: 0;
    bottom: 0;
}

This results in a 6985x7505 image. I'm not sure why it is getting continually uploaded however, sometimes in similar bugs it's due to the height changing by a pixel due to snapping when scrolling, but that doesn't appear to be the case here.

I'll look in to why the image keeps getting uploaded. But bug 1673653 should help with this by making the svg use the blob image path.

Comment 3

[Jamie Nicol [:jnicol]]

The huge image keeps getting evicted from the texture cache during evict_items_from_cache_if_required(). It was requested on the previous frame, and gets requested again on the same frame as it has been evicted, meaning we must re-upload it. This happens on most frames. (Sometimes it is the 33rd+ item in the LRU cache so is lucky enough to not be evicted, but other smaller items will still get evicted and reuploaded.)

Glenn, should we modify it so that we do not evict items which were used on the previous frame? Or could we even call evict_items_from_cache_if_required() after we have done all our requests, but before the updates, and not evict items which were requested on the current frame?

Comment 4

[Jamie Nicol [:jnicol]]

FTR I cannot reproduce on my desktop, but there's nothing android specific about the underlying issue here. Perhaps it's just the version of the page the website serves us, or the display resolution affecting the size of the image.

Comment 5

[Glenn Watson [:gw]]

Yes, that sounds reasonable.

It's slightly tricky to change the call order and/or explicitly avoid evicting items from the previous frame (it is possible though).

But there might be an easier way to achieve this goal. What if we change should_continue_evicting so that instead of using a fixed memory threshold, we adjust that threshold based on the used amount of texture memory on the previous frame? (We could probably track this fairly easily by incrementing a counter for each unique texture request during the frame). Does that sound like a reasonable option?

Comment 6

[Glenn Watson [:gw]]

Or, another thought - what about if we keep an estimate of how many textures are being regularly evicted and re-uploaded each frame, and when that is high we increase the eviction threshold amount, or something like that?

Comment 7

[Jamie Nicol [:jnicol]]

I'm not sure I understand your first suggestion, could you expand on it a bit? For the second suggestion, if I understand correctly, my worry would be that there would be some very bad frames before the system reacts appropriately.

Comment 8

[Glenn Watson [:gw]]

For the first suggestion, it would be something similar to:

TextureCache::request {
    if !texture_requested_this_frame_already {
        self.used_memory += texture.size;
    }
}

And then instead of a constant EVICTION_THRESHOLD_SIZE in should_continue_evicting we'd use the used_memory as a threshold, so that if the working set size is, say, 128M, that becomes our threshold after which we stop evicting items, rather than the current hard-coded 64M.

Haven't thought it through too much, but I think that would work OK (not sure if it's actually easier / simpler than your original suggestion above though, will probably need to prototype whichever option you prefer and see).

Comment 9

[Jamie Nicol [:jnicol]]

Ah I understand now, that makes sense. It was actually very easy to do my original idea of not evicting items used on the previous frame. Let me know if you don't like that though and I can prototype the alternative.

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Bug 1676909 - Don't evict items from the texture cache if they were used in the previous frame. r?gw

Webrender uses an LRUCache to hold the items in the texture
cache. When texture usage is over a certain threshold we evict the
least recently used items until we are back under the
threshold. However, this runs in to a problem on pages where the cache
only contains items still in use, but is still over the threshold, as
even the least recently used item is still required. In this scenario
we end up evicting items at the start of the frame, only to reupload
them later in the frame, and repeat the cycle again on the next frame.

To avoid this, tweak the eviction algortithm so that it never evicts
items that were in use in the previous frame. (The eviction step
occurs before we know which items are needed for the current frame, so
using the previous frame is the best approximation.)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ee685602a7f
Don't evict items from the texture cache if they were used in the previous frame. r=gw

Comment 12

[Jamie Nicol [:jnicol]]

The patch just landed fixes the problem for the most part. There is no longer texture upload on basically every frame when scrolling. However, as soon as the giant texture is not used for a frame then it will be evicted. This occurs when scrolling to the very bottom of the page, then scrolling back up.

I think bug 1673653 will help with this: by making the SVG a tiled blob image we should have to only upload a few tiles instead of the whole image.

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0ee685602a7f