Store a queue of rasterized blobs instead for non-tiled images.

[nical]

This fixes a race-condition that happens if we don't upload rasterized blobs right after scene building which blocks the work on avoiding redundant frame builds.

The idea is that instead of keeping a single rasterized blob per image, we store a queue which lets us have several partial updates of a blob in flight (previously the second update would clobber the first and miss the updated area of the first update).
To prevent the queue from growing indefinitely, we make it so that older elements are discarded if a newer one covers them, and re-rasterize everything if the queue gets big to put it back to a single update.

I expect that most of the time for visible items the queue will stay at 1 (or rarely 2) updates even if the blob is updating constantly because we tend to render at least as often as we build scenes.
For items that are off-screen and thus not getting uploaded, the queue will alternate between 1, 2 and 3 elements, and we'll force the rasterization of the whole image each time the queue gets to 3 to force the queue back to 1 element.

The case of animated off-screen blobs is pretty bad because they generate rasterization constantly and don't end up on screen, but that's already the case today.
We could (as a followup) try to just discard the elements of the queue when it gets big (3 elements or more) and skip rasterization of the blob image until we see it.
This means the next time we see it we'll risk jank since it will trigger the synchronous rasterization during frame building, but at least we'd have avoided rasterizing the blobs during the time it was animating off-screen.

---

This change is 

[nical]

This PR might be easier to understand by reading the commits individually. It looks kinda complicated and I'm sorry about that. The alternative was in my opinion also complicated and more invasive.
I'm still testing it out in conjunction with the frame building stuff.

[kvark]

Looks good in general (and good job discovering the race condition!). I left a few questions below. Also needs a try push.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nical)

---

webrender/src/resource_cache.rs, line 610 at r1 (raw file):

            let image = self.rasterized_blob_images.entry(request.key).or_insert_with(
                || { RasterizedBlob::Tiled(FastHashMap::default()) }

shouldn't this depend on the request.tile? i.e. why do we create Tiled here specifically?

---

webrender/src/resource_cache.rs, line 615 at r2 (raw file):

            if let Some(tile) = request.tile {
                if let RasterizedBlob::NonTiled(..) = *image {

why aren't we just matching?

---

webrender/src/resource_cache.rs, line 1425 at r2 (raw file):

            debug_assert!(image_template.data.uses_texture_cache());

            let mut updates: SmallVec<[(ImageData, Option<DeviceUintRect>); 1]> = SmallVec::new();

can we just keep it inself as a temporary reusable array?

---

webrender/src/resource_cache.rs, line 1447 at r2 (raw file):

                            for img in queue.drain(..) {
                                updates.push((
                                    ImageData::Raw(Arc::clone(&img.data)),

can probably skip the Arc::clone here for `img being moved in

---

webrender/src/resource_cache.rs, line 1113 at r3 (raw file):

                // We do the latter here but it's not ideal and might want to revisit and do
                // the former instead.
                match self.rasterized_blob_images.get_mut(&key) {

shouldn't need get_mut here, also can just go with if let given that you ignore the other arms anyway

[nical]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nical)

---

webrender/src/resource_cache.rs, line 610 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't this depend on the request.tile? i.e. why do we create Tiled here specifically?

This is just a placeholder, we have to create the entry with something and below we populate the thing (making it non-tiled if need be).

---

webrender/src/resource_cache.rs, line 615 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why aren't we just matching?

Here we first look at whether the thing is non-tiled and make it tiled. Then as a second step we push the tiles. if we were to try to do it on a single match we would have to at the same time get a mutable handle on the tiles in the ::Tiled arm and be able to overwrite the whole thing on the other arm, and have both arms push the tiles.
I find it just simpler this way.

---

webrender/src/resource_cache.rs, line 1425 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we just keep it inself as a temporary reusable array?

Is that really simpler? having a smallvec on the stack feels like less work to me since we only use it here and that way we don't need to think about whether state can accidentally persist or anything.

---

webrender/src/resource_cache.rs, line 1447 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can probably skip the Arc::clone here for `img being moved in

Ah yes indeed.

[nical]

This try push has the patches from this PR plus the backed out ones that prevent redundant frame building
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f436bfb2b982214738e512042a460c73bc96432
It looks good so far.

[kvark]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kvark and @nical)

---

webrender/src/resource_cache.rs, line 610 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

This is just a placeholder, we have to create the entry with something and below we populate the thing (making it non-tiled if need be).

I see. A comment would help :)

---

webrender/src/resource_cache.rs, line 1425 at r2 (raw file):

Previously, nical (Nicolas Silva) wrote…

Is that really simpler? having a smallvec on the stack feels like less work to me since we only use it here and that way we don't need to think about whether state can accidentally persist or anything.

it's a simple guarantee of no allocations in place (on regular basis, of course), where SmallVec is more of a hack in place to fix some of the common cases

---

webrender/src/resource_cache.rs, line 1447 at r2 (raw file):

Previously, nical (Nicolas Silva) wrote…

Ah yes indeed.

still there

---

webrender/src/resource_cache.rs, line 1113 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't need get_mut here, also can just go with if let given that you ignore the other arms anyway

ping

[nical]

Yep I haven't addressed the nits yet, coming soon.
I don't think of SmallVec as hack. I wish rust was better at expressing them but it's probably my favorite data structure.
Persisting a vector however (when in the mast majority of cases the size will be 1) feels like a more convoluted solution to the problem.

[kvark]

My general concern about SmallVec is that it makes it more difficult to reason about performance. If you increase the scene complexity gradually, you don't want the frame rate to jump rapidly. In case of SmallVec, there is a threshold upon crossing which you end up paying for the heap allocation on every use. With a persistent temporary vector, there is no such problem.

This particular case may not be that important, I'm just expressing the general idea.

[nical]

This is specific case is independent of scene complexity and is only requiring the allocation in rare cases of unfortunate timings. I see where you are coming from but in this case I believe SmallVec is sensible.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nical)

[nical]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @kvark and @nical)

---

webrender/src/resource_cache.rs, line 1447 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

still there

Done.

---

webrender/src/resource_cache.rs, line 1113 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

ping

Done.

[kvark]

Reviewed 1 of 1 files at r5, 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

[kvark]

shipit
@bors-servo r+

[bors-servo]

📌 Commit 0cf32bf has been approved by kvark

[bors-servo]

⌛ Testing commit 0cf32bf with merge ed8766b...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing ed8766b to master...