Initial refactor of some webrender types so that prim instances are immutable
Categories
(Core :: Graphics: WebRender, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox152 | --- | fixed |
People
(Reporter: gw, Assigned: gw)
References
Details
(Keywords: regression)
Attachments
(23 files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
|
Assignee | |
Comment 1•2 months ago
|
||
Partition PrimitiveScratchBuffer's flat field list into three sub-structs
based on clear/recycle behavior, so a future edit can't accidentally add
a per-frame field to per-scene storage (or vice versa).
- PrimitiveFrameScratch: cleared in begin_frame (line_decoration,
normal_border, border_task_ids, clip_mask_instances, debug_items,
required_sub_graphs, quad_{direct,indirect}_segments). - PrimitiveSceneCache: only recycled on scene rebuild (glyph_keys,
segments, segment_instances). Future patch will migrate these
to per-frame; this struct may shrink or dissolve then. - PrimitiveRetained: lives across both frames and scenes with its own
trim policy (messages, quad_tile_classifier).
Also drops quad_color_segments, which was added in c696e4cd2e1a5 but
has never had a write or read site.
|
||
Updated•2 months ago
|
|
|
Assignee | |
Comment 2•2 months ago
|
||
Drops the instance.kind stutter. PrimitiveInstanceIndex stays since it
references the instance, not the kind.
Mechanical rename with no behavior change.
|
|
Assignee | |
Comment 3•2 months ago
|
||
Also renames VisibilityState -> DrawState and PrimitiveInstance.vis ->
.draw. The struct already contains more than visibility (clip chain,
clip task index); the new name reflects what it actually is — the
central per-draw record that command buffers will index in follow ups.
|
|
Assignee | |
Comment 4•2 months ago
|
||
PicturePrimitive -> PictureInstance
TextRunPrimitive -> TextRunInstance
SegmentedInstance -> BrushSegmentation
|
|
Assignee | |
Comment 5•2 months ago
|
||
Migrate per-frame fields off four retained primitive instance types
into sub-storages on PrimitiveFrameScratch, keyed by an Index carried
on the corresponding PrimitiveKind variant. Prepare and batch read
from the scratch entry instead of fields on the retained instance.
- BackdropRenderScratch { src_task_id } removes the cross-kind batch
read of the source Picture's primary_render_task_id. - PictureScratch { primary_render_task_id, secondary_render_task_id,
extra_gpu_data } empties PictureInstance of its three per-frame
fields. Picture re-entry uses a picture_scratch_handles slot on
FrameBuildingState; prepare_picture returns
Option<Index<PictureScratch>>. - ImageScratch { visible_tiles, src_color, normalized_uvs, adjustment }
drops the per-instance visible_tiles Vec that allocated every frame;
tile entries land in a shared visible_image_tiles storage referenced
by Range. - TextRunScratch { used_font, glyph_keys_range,
snapped_reference_frame_relative_offset, raster_scale }, plus
glyph_keys graduates from PrimitiveSceneCache to per-frame storage.
Combined because per-frame glyph_keys_range with per-scene glyph_keys
would leak between scene rebuilds; cache_dirty memoization is
dropped, keys rebuilt per frame for visible runs.
PrimitiveSceneCache now holds only segments and segment_instances.
|
|
Assignee | |
Updated•2 months ago
|
|
|
Assignee | |
Comment 6•2 months ago
|
||
Migrate the per-frame draw header (visibility state, clip chain, clip
task index) off PrimitiveInstance.draw into a parallel
scratch.frame.draws: Vec<PrimitiveDrawHeader> on PrimitiveFrameScratch,
identity-indexed by PrimitiveInstanceIndex.0 and resized to
prim_instances.len() at frame start. Discharges the TODO(gw) on the
draw field — PrimitiveInstance is now strictly scene-retained:
{kind, clip_leaf_id, prim_origin}.
Notable mechanics:
- New prim_instance_index back-reference on PrimitiveDrawHeader (with
PrimitiveInstanceIndex::INVALID const) lets readers reach the prim
instance in a follow-up that switches storage to push-per-draw. - prepare_interned_prim_for_render snapshots the draw header into a
prim_info local; PrimitiveDrawHeader is Copy now to make this cheap
and avoid a &mut scratch borrow conflict. - update_clip_task, update_prim_dependencies, and ImageData::update
gain prim_instance_index parameters and read/reset scratch directly
instead of taking &mut PrimitiveDrawHeader. - PictureInstance::restore_context drops its unused prim_instances
parameter and gains &PrimitiveScratchBuffer for the
cmd-buffer-targets read. - PrimitiveInstance::reset / clear_visibility go away with the field.
|
|
Assignee | |
Comment 7•2 months ago
|
||
Move the per-kind scratch index off PrimitiveKind variants
(LineDecoration, NormalBorder, Image, TextRun, Picture, BackdropRender)
onto a new kind_scratch: KindScratchHandle field on PrimitiveDrawHeader.
PrimitiveKind is now strictly scene-stable identity handles — there is
no remaining per-frame mutable state on the variant.
Mechanics:
- KindScratchHandle is an enum with one variant per scratch-bearing
kind plus None. Per-kind unwrap_X() helpers extract the index;
callers know the variant by construction from the matching
PrimitiveKind arm, so the panic path is unreachable in practice. - Prepare sites write the kind_scratch slot on
scratch.frame.draws[idx]; readers in batch.rs and prepare.rs source
the index from there. Variant constructors and scene_building drop
the scratch_handle: Index::INVALID initializer; the field is removed.
Also types cmd buffer entries as storage::Index<PrimitiveDrawHeader>
instead of PrimitiveInstanceIndex. The numerical value is unchanged
(scratch.frame.draws is still identity-indexed by
PrimitiveInstanceIndex.0); the rename locks in the invariant that cmd
buffer entries identify draws, not instances. storage::Index gains a
public from_u32 constructor and .0 accessor so external producers can
mint typed handles for the still-Vec-backed draws storage.
|
|
Assignee | |
Comment 8•2 months ago
|
||
Three related migrations:
-
segments + segment_instances move from PrimitiveSceneCache to
PrimitiveFrameScratch. The INVALID-sentinel skip in
build_segments_if_needed is gone — segments are rebuilt fresh for
every visible segmented prim every frame. The segment builder now
runs unconditionally. -
segment_instance_index moves off PrimitiveKind::Rectangle / YuvImage
variants and off ImageInstance. It now lives on PrimitiveDrawHeader
as a single per-draw field — a unified handle for all three kinds
that opt into segmented brush rendering. Initialized to UNUSED
(not INVALID — the "needs build" sentinel has no meaning now that
we always build). -
PrimitiveSceneCache is reduced to an empty placeholder; the only
retained-across-frames primitive scratch is PrimitiveRetained
(debug log + tile classifier). The empty PrimitiveSceneCache stays
for now to preserve the lifetime split at the type level; a
follow-up can drop it entirely if desired.
Notable mechanics:
- build_segments_if_needed gains a prim_instance_index parameter and
takes scratch directly; writes the new index to
scratch.frame.draws[idx].segment_instance_index. - update_clip_task_for_brush takes a new prim_segment_instance_index
parameter; its now-unused prim_store parameter is dropped. - Readers (Rectangle/YuvImage/Image arms in prepare and batch) read
prim_info.segment_instance_index and scratch.frame.segments /
segment_instances. - debug_asserts against SegmentInstanceIndex::INVALID drop; INVALID
is no longer reachable in the new flow.
|
|
Assignee | |
Comment 9•2 months ago
|
||
ImageInstance held a single field, tight_local_clip_rect, written
each frame from the prim's clip chain. Move it into ImageScratch
and drop the surrounding indirection: the ImageInstance struct, the
prim_store.images storage, image_instance_index on
PrimitiveKind::Image, and image_count on PrimitiveStoreStats all go.
ImageData::update no longer takes &mut ImageInstance — it writes the
clip rect onto the freshly-built ImageScratch entry. Readers in
prepare and batch drop the prim_store.images lookup. The stale
TODO(gw) on Image::make_instance_kind is discharged.
|
|
Assignee | |
Comment 10•2 months ago
|
||
shadow and requested_raster_space already participate in TextRunKey's
interning identity, but the template dropped them on intern and
TextRunInstance carried a per-instance copy. Move them into
TextRunTemplate (populated by From<TextRunKey>) and drop the
TextRunInstance indirection: TextRunInstance struct, TextRunStorage,
TextRunIndex, prim_store.text_runs, and text_run_count are gone.
run_index is removed from PrimitiveKind::TextRun.
compute_font_instance, get_raster_space_for_prim, and
request_resources move from TextRunInstance to TextRunTemplate.
request_resources drops its now-redundant specified_font and glyphs
parameters. Readers in prepare and batch drop the
prim_store.text_runs lookup.
TextRunTemplate grows 88 → 96 bytes — paid once per dedup'd template
rather than per instance.
|
|
Assignee | |
Comment 11•2 months ago
|
||
color_binding_index was pure redundancy. RectangleTemplate already
holds the color (PropertyBinding<ColorF>), and its only consumer —
the tile-cache invalidation tracker — already reads the template
two lines above the color_bindings lookup. The parallel
prim_store.color_bindings arena existed only to duplicate the same
binding identity in a different value type. Color resolution for
rendering goes through RectangleTemplate::update, which is unchanged.
Plumbing dropped: color_binding_index on PrimitiveKind::Rectangle,
prim_store.color_bindings, ColorBindingIndex/Storage typedefs,
color_binding_count on PrimitiveStoreStats, and the color_bindings
parameter on update_prim_dependencies. The tile-cache Rectangle arm
derives the binding identity from the template via a
PropertyBinding<ColorF>.into().into() chain — the binding key.id is
preserved.
|
|
Assignee | |
Comment 12•2 months ago
|
||
compositor_surface_kind on PrimitiveKind::{Image, YuvImage} was
per-frame mutable state hiding inside a scene-stable variant — the
tile-cache promotion logic in update_prim_dependencies rewrites it
each frame depending on whether this draw should be a Blit, Underlay,
or Overlay. Same anti-pattern Pt 6 cleaned up for
PrimitiveInstance.draw.
Move it onto PrimitiveDrawHeader, alongside the other per-draw
decisions (state, clip_chain, clip_task_index, segment_instance_index).
Initialized to Blit; set during the visibility pass; read via
prim_info snapshots in prepare and batch.
PrimitiveKind::Image and PrimitiveKind::YuvImage shrink to
{ data_handle } — pure scene-stable identity. Every PrimitiveKind
variant is now strictly identity (no per-frame mutable state on the
enum).
update_prim_dependencies converts its long-lived prim_clip_chain
borrow to a Copy snapshot so the new mutable writes can coexist.
|
|
Assignee | |
Comment 13•2 months ago
|
||
Move NormalBorderData's brush_segments + border_segments to per-frame
scratch (NormalBorderScratch + new arenas on PrimitiveFrameScratch),
built each frame by NormalBorderScratch::build_for_prim ahead of
update_clip_task. Storage gains data_mut() for direct interleaved push,
and a debug-only open_count assertion to catch unmatched
open_range/close_range pairs.
NormalBorderTemplate size 208 -> 152.
|
|
Assignee | |
Comment 14•2 months ago
|
||
Same pattern as Pt 13: brush_segments moves from ImageBorderData to a
new ImageBorderScratch + arena, built each frame by
ImageBorderScratch::build_for_prim. The nine_patch descriptor moves
onto the template. NormalBorder + ImageBorder clip-task arms unify
onto a single prim_brush_segments_range parameter.
ImageBorderTemplate size 96 -> 112.
|
|
Assignee | |
Comment 15•2 months ago
|
||
Same pattern as Pt 13/14: brush_segments moves from
LinearGradientTemplate to a new LinearGradientScratch + arena. Only
nine-patch gradients allocate scratch; build_for_prim is a no-op when
border_nine_patch is None, and the clip-task resolver matches on
kind_scratch directly. NormalBorder + ImageBorder + LinearGradient
clip-task arms collapse onto one match.
LinearGradientTemplate size 128 -> 104.
|
|
Assignee | |
Comment 16•2 months ago
|
||
Replace the three retained shadow rects on the BoxShadow key/template
with the underlying parameters (box_offset, signed spread_amount) and
derive inner/outer/element per-frame in prepare from prim_rect plus
the parameters and blur_radius. BoxShadow key shrinks by 36 bytes.
|
|
Assignee | |
Comment 17•2 months ago
|
||
Linear/Radial/Conic gradient templates each carry task_size and scale
fields (set in From<*Key>) that have had no readers since the legacy
gradient render-task cache was removed (no RenderTaskCacheKeyKind for
gradients; PatternBuilder::build for radial explicitly uses
no_scale = (1,1)). Drop the fields and the From-impl code that
computed them, including the conic hard-stops detection that only
fed max_size for the dead clamp.
Template sizes shrink by 16 bytes each (LinearGradient 104 -> 88;
Radial 112 -> 96; Conic 112 -> 96).
|
|
Assignee | |
Comment 18•2 months ago
|
||
All LinearGradient prims dispatch to the quad path, so the per-frame
brush-segment scratch introduced in Pt 15 has no consumer.
Removes LinearGradientScratch, the linear_gradient arena on
PrimitiveFrameScratch, the KindScratchHandle::LinearGradient variant,
and the LinearGradient arms in update_clip_task[_for_brush]. The
latter joins Radial/Conic on unreachable!. border_nine_patch stays —
quad::prepare_border_image_nine_patch reads it.
|
||
Comment 19•2 months ago
|
||
|
|
Assignee | |
Comment 20•2 months ago
|
||
Replace the absolute stretch_size on Linear/Radial/Conic gradient keys
and templates with a per-axis stretch_ratio (= stretch_size / prim_size,
clamped to [0, 1]). At prepare time, derive stretch_size as
stretch_ratio * common.prim_size at the six gradient prep sites. Drop
the now-redundant .min(prim_size) clamp from RadialGradientTemplate::from
and ConicGradientTemplate::from -- the clamp is baked into the ratio.
|
|
Assignee | |
Comment 21•2 months ago
|
||
Image and NormalBorder gain the flag on their per-kind scratch (set
in update / build_for_prim respectively). ImageBorder is hard-coded
true at the batch reader (always wants repetition-capable shader).
Other prim kinds never reach the brush_image consumer of
BatchFeatures::REPETITION, so their dead writes are dropped along
with prim_may_need_repetition and the field itself.
|
||
Comment 22•2 months ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 23•2 months ago
|
||
Drop the cache key from the LineDecoration intern key; the mask tile size
is a deterministic function of style/orientation/thickness and the prim's
local size, so derive it in prepare instead of baking it in at scene
build.
|
|
||
Comment 24•2 months ago
|
||
|
|
Assignee | |
Comment 25•2 months ago
|
||
Move the GPU buffer address written by NormalBorderData::write_brush_gpu_blocks
and ImageBorderData::update off the shared common.gpu_buffer_address onto a
per-instance gpu_address slot on NormalBorderScratch / ImageBorderScratch.
Both write blocks whose contents (stretch_size, brush segments) depend on
the prim's per-instance size, so once intern keys can dedup across sizes,
multiple instances of the same template will need their own addresses.
Other prim kinds (Image, YuvImage, Rectangle, TextRun, LineDec, gradients)
keep common.gpu_buffer_address — their GPU blocks are template-stable.
|
|
Assignee | |
Comment 26•2 months ago
|
||
Replace PrimitiveInstance.prim_origin: LayoutPoint with prim_rect: LayoutRect
and drop prim_size from PrimKeyCommonData and PrimTemplateCommonData. All
template-side readers now source the per-instance size from prim_instance
.prim_rect.size(); intern keys for prims that differ only in size now
share a single template entry, with per-instance GPU addresses (Pt 22)
carrying the size-dependent block bytes correctly.
|
|
||
Comment 27•2 months ago
|
||
|
||
Comment 28•2 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/798eb91e622c
https://hg.mozilla.org/mozilla-central/rev/8751f391e9d4
https://hg.mozilla.org/mozilla-central/rev/eb74aad4ea61
https://hg.mozilla.org/mozilla-central/rev/0ea2c67a767c
https://hg.mozilla.org/mozilla-central/rev/a2a5290999d3
https://hg.mozilla.org/mozilla-central/rev/b2e8a80208ed
|
|
||
Comment 29•2 months ago
|
||
|
|
||
Comment 30•2 months ago
|
||
| bugherder | ||
|
|
||
Comment 31•2 months ago
|
||
|
|
||
Comment 32•2 months ago
|
||
| bugherder | ||
|
|
||
Comment 33•2 months ago
|
||
|
|
||
Comment 34•2 months ago
|
||
|
||
Comment 35•2 months ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•1 month ago
|
|
||
Updated•1 month ago
|
|
||
Updated•18 days ago
|
|
|
||
Updated•18 days ago
|
Description
•