Various render task refactors to help with upcoming caching changes. #2663

glennw · 2018-04-16T03:18:53Z

request_render_task() now returns a handle to a render task
cache entry, rather than an allocated texture cache item. This
is more in line with how normal texture cache request work.
Allows the allocation to happen later in the frame (for example
if the exact render task size is not yet known).
Allows storing more information about a cached render task
than what a texture cache item stores (for example, a specific
blend mode that is required).
Allows batching code to cheaply look up a cached render task
entry by using a freelist instead of a hashmap. This will become
more important as we start to cache render tasks that have
larger / more complex cache keys.
Add an extra type parameter to freelist.rs to allow using a custom
marker type. This avoids rustc errors related to Send or Sync
not being implemented on free list handles with !Send or !Sync
phantom data types.
Allow the size of a render task to be set to None initially, if
it is not known.
Add a new render task call (prepare_for_render) which can be used
by render tasks to prepare tasks that rely on glyphs and/or images
in the resource cache to be resolved first. This can also be used
to set the size of a render task before render target allocation,
if the render task size is not known during initial visibility pass.

This change is

glennw · 2018-04-16T03:20:18Z

Try run here - looks good from what I can tell (the orange appear to be unrelated or expected differences based on current master).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4178091e564962ba1619ab2f2523d93b204150a&selectedJob=173826819

The changes here are a bit opaque in ~~~purpose~~~ places, but I'm trying to make changes incrementally so the reviews are a bit easier. If you want to discuss in detail the reasoning for some of these items, or hold off until the upcoming PRs are ready, that's fine.

kvark · 2018-04-16T20:02:52Z

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

webrender/src/clip.rs, line 23 at r1 (raw file):

#[derive(Debug)]
pub struct ClipStoreMarker;

nit: should be an enum ClipStoreMarker {} to indicate that it's not meant to be instanciated

webrender/src/picture.rs, line 325 at r1 (raw file):

                let surface = if pic_state_for_children.has_non_root_coord_system {
                    let picture_task = RenderTask::new_picture(
                        RenderTaskLocation::Dynamic(None, Some(device_rect.size)),

when is the size ever None? I suppose you'll have some cases in the future, but not yet?

webrender/src/prim_store.rs, line 1212 at r1 (raw file):

                                    // Size in device-pixels we need to allocate in render task cache.
                                    size: texel_rect.size,
                                    item: CacheItem::invalid(),

can we get rid of invalid() now? or is it still being used by other things?

webrender/src/render_task.rs, line 899 at r1 (raw file):

#[derive(Debug)]
pub struct RenderTaskCacheMarker;

same here (re:enum)

webrender/src/render_task.rs, line 1057 at r1 (raw file):

        if cache_entry.pending_render_task_id.is_none() {
            // Check if this texture cache handle is valid.
            if texture_cache.request(&cache_entry.handle, gpu_cache) {

what if it's not valid? seems wrong to just ignore it here

Comments from Reviewable

* request_render_task() now returns a handle to a render task cache entry, rather than an allocated texture cache item. This is more in line with how normal texture cache request work. * Allows the allocation to happen later in the frame (for example if the exact render task size is not yet known). * Allows storing more information about a cached render task than what a texture cache item stores (for example, a specific blend mode that is required). * Allows batching code to cheaply look up a cached render task entry by using a freelist instead of a hashmap. This will become more important as we start to cache render tasks that have larger / more complex cache keys. * Add an extra type parameter to freelist.rs to allow using a custom marker type. This avoids rustc errors related to Send or Sync not being implemented on free list handles with !Send or !Sync phantom data types. * Allow the size of a render task to be set to None initially, if it is not known. * Add a new render task call (prepare_for_render) which can be used by render tasks to prepare tasks that rely on glyphs and/or images in the resource cache to be resolved first. This can also be used to set the size of a render task before render target allocation, if the render task size is not known during initial visibility pass.

glennw · 2018-04-16T20:55:02Z

Review status: 8 of 11 files reviewed at latest revision, 5 unresolved discussions.

webrender/src/clip.rs, line 23 at r1 (raw file):