Skip to content

Commit 1efb73b

Browse files
lhkbobskcq-be@skia-corp.google.com.iam.gserviceaccount.com
lhkbob
authored and
skcq-be@skia-corp.google.com.iam.gserviceaccount.com
committed
[graphite] Convert searchBackwards loop to simple for-loop
As searchBackwards() is simplified, the `processLayer` lambda with capturing stack parameters becomes more of an obfuscation of the looping logic. This also introduces some local variables to prepare for improving the BoundsTest enum's values. Bug: 419535595 Change-Id: I9494870d1ac488ff131a5e62083255da35e82bc4 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1283279 Reviewed-by: Thomas Smith <thomsmit@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>
1 parent 731888c commit 1efb73b

1 file changed

Lines changed: 74 additions & 118 deletions

File tree

‎src/gpu/graphite/DrawListLayer.cpp‎

Lines changed: 74 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -62,133 +62,88 @@ std::pair<Layer*, BindingList*> DrawListLayer::searchBackwards(
6262
BindingList* forwardMerge = nullptr;
6363

6464
Layer* current = fLayers.tail();
65-
int32_t limit = kMaxSearchLimit;
66-
auto processLayer = [&]() -> bool {
67-
auto [overlapType, match] =
68-
isStencil
69-
? current->test</*kIsStencil=*/true>(
70-
isDepthOnly,
71-
drawParams->drawBounds(),
72-
key,
73-
requiresBarrier)
74-
: current->test</*kIsStencil=*/false>(
75-
isDepthOnly,
76-
drawParams->drawBounds(),
77-
key,
78-
requiresBarrier);
79-
80-
if (overlapType == BoundsTest::kIncompatibleOverlap) {
81-
// If we need to read the dst, we cannot go earlier than this layer.
82-
if (dependsOnDst) {
83-
// Forward merging attempts to pull an earlier, compatible draw out of the
84-
// current layer and push it into a newly created layer to improve
85-
// pipeline/texture batching.
86-
//
87-
// 1. Draw Type Restrictions (Single Renderstep & No Depth-Only):
88-
// Forward merging is strictly limited to single-renderstep shading draws. We
89-
// explicitly forbid depth-only draws (which pass `false` for
90-
// `canForwardMerge`), and the single-step requirement inherently excludes
91-
// stencil draws. If we allowed multi-step renderers to forward merge, we
92-
// would risk pulling a parent renderstep forward and over its
93-
// already-inserted child.
94-
//
95-
// 2. Directional & Spatial Validity:
96-
// Because we evaluate bindings backwards (tail to head), any binding matches
97-
// prior to intersection are necessarily execute *after* that intersecting
98-
// draw. Furthermore, because standard shading draws within the same layer
99-
// are guaranteed by the `test()` logic to be mutually disjoint, the matched
100-
// draw does not overlap with any of the later bindings we evaluated and
101-
// skipped. Therefore, it is visually safe to extract this disjoint match and
102-
// defer its execution to a new, subsequent layer without violating the
103-
// Painter's Algorithm.
104-
//
105-
// 3. The Tail-Only Restriction: We strictly limit forward merging to the *tail*
106-
// of the layer list. If we allowed forward merging from a middle layer, we
107-
// would be forced to insert the newly generated target layer into the middle
108-
// of the list. This would break the structural invariant that
109-
// `Layer::fOrder` strictly increases with the physical list order.
110-
//
111-
// 4. The Clip State Complication (Drawn/Undrawn Mix):
112-
// While depth-only draws never forward merge themselves, allowing forward
113-
// merging to middle-insert layers risks clip stack ordering issues. The clip
114-
// stack relies on the `CompressedPaintersOrder` invariant when processing a
115-
// mix of drawn and undrawn elements. `updateClipStateForDraw` uses
116-
// `Insertion::operator>` (which compares `fOrder`) to find the latest
117-
// insertion across all depth-only clips affecting a draw. If a layer were
118-
// middle-inserted via `addAfter`, assigning it a valid `fOrder` is
119-
// intractable:
120-
// - Case A (New Highest Order): If we give the middle layer the next
121-
// highest integer (e.g., L1(1) -> L_mid(3) -> L2(2)), the `max()`
122-
// calculation incorrectly flags `L_mid` as the absolute latest boundary.
123-
// A draw depending on a clip in `L2` will incorrectly take `L_mid` as
124-
// its stop layer and bypass its actual stop layer `L2`.
125-
// - Case B (Duplicate Order): If we duplicate the order to avoid Case A
126-
// (e.g., L1(1) -> L2(2) -> L2b(2)), the tie-breaker math breaks. If Clip
127-
// A inserts into `L2` and Clip B inserts into `L2b`, `max(A, B)` cannot
128-
// distinguish them because `2 > 2` is false. Depending on iteration
129-
// order, it may incorrectly return `L2` as the boundary, causing the
130-
// clipped draw to execute before Clip B's mask is rendered. Restricting
131-
// forward merges to the tail guarantees our assigned ordering is always
132-
// valid.
133-
if (match && current == fLayers.tail() && canForwardMerge) {
134-
if (current->fBindings.head() != current->fBindings.tail() &&
135-
(!requiresBarrier ||
136-
!match->fBounds.intersects(drawParams->drawBounds()))) {
137-
forwardMerge = match;
138-
targetMatch = forwardMerge;
139-
}
140-
}
141-
return true;
142-
}
143-
// If !dependsOnDst, simply keep searching backwards. Since we guarantee stencil
144-
// atomicity to a layer, a stencil intersection can still safely bypass this layer,
145-
// because an insertion downstream cannot disrupt this stencil region.
146-
return false;
147-
} else {
148-
// Found a valid layer (Compatible or Disjoint)
65+
for (int limit = kMaxSearchLimit; limit > 0 && current; --limit) {
66+
auto [overlapType, match] = isStencil ?
67+
current->test</*kIsStencil=*/true>(isDepthOnly,
68+
drawParams->drawBounds(),
69+
key,
70+
requiresBarrier) :
71+
current->test</*kIsStencil=*/false>(isDepthOnly,
72+
drawParams->drawBounds(),
73+
key,
74+
requiresBarrier);
75+
76+
const bool allowedInLayer = overlapType == BoundsTest::kDisjoint ||
77+
overlapType == BoundsTest::kCompatibleOverlap;
78+
79+
const bool allowedBeforeLayer =
80+
overlapType == BoundsTest::kDisjoint ||
81+
(!dependsOnDst && overlapType == BoundsTest::kIncompatibleOverlap);
82+
83+
// Forward merging attempts to pull an earlier, compatible draw out of the
84+
// current layer and push it into a newly created layer to improve
85+
// pipeline/texture batching.
86+
//
87+
// 1. Draw Type Restrictions (Single Renderstep & No Depth-Only):
88+
// Forward merging is strictly limited to single-renderstep shading draws. We
89+
// explicitly forbid depth-only draws (which pass `false` for
90+
// `canForwardMerge`), and the single-step requirement inherently excludes
91+
// stencil draws. If we allowed multi-step renderers to forward merge, we
92+
// would risk pulling a parent renderstep forward and over its
93+
// already-inserted child.
94+
//
95+
// 2. Directional & Spatial Validity:
96+
// Because we evaluate bindings backwards (tail to head), any binding matches
97+
// prior to intersection are necessarily execute *after* that intersecting
98+
// draw. Furthermore, because standard shading draws within the same layer
99+
// are guaranteed by the `test()` logic to be mutually disjoint, the matched
100+
// draw does not overlap with any of the later bindings we evaluated and
101+
// skipped. Therefore, it is visually safe to extract this disjoint match and
102+
// defer its execution to a new, subsequent layer without violating the
103+
// Painter's Algorithm.
104+
//
105+
// 3. The Tail-Only Restriction: We strictly limit forward merging to the *tail*
106+
// of the layer list. If we allowed forward merging from a middle layer, we
107+
// would be forced to insert the newly generated target layer into the middle
108+
// of the list. This would break the structural invariant that
109+
// `Layer::fOrder` strictly increases with the physical list order; this invariant
110+
// necessary to ensure that a draw is inserted after *ALL* depth-only clip draws
111+
// that affect it.
112+
canForwardMerge &= current == fLayers.tail() && !requiresBarrier;
113+
114+
if (allowedInLayer) {
115+
// Allowed in the layer, so remember it. In complex scenes, we want to search deeper
116+
// in the layer list than just the first compatible overlap we encounter. Stopping early
117+
// reduces search time but fragments batching. Inserting early blocks subsequent draws
118+
// from reaching those denser, later candidates (particularly when this is a clip draw
119+
// as that propagates into the stop layer for subsequent draws).
149120
targetLayer = current;
150121
targetMatch = match;
151-
152-
// In stencil-heavy scenes, we want to search deeper into the list than the first
153-
// compatible overlap we encounter. An earlier match likely contains fewer draws and
154-
// less draw coverage, while a later match is likely denser. Stopping at the first
155-
// match minimizes search time but fragments batching. Inserting early carries a
156-
// dual penalty: it 1) blocks subsequent draws from reaching those denser, later
157-
// candidates, and it 2) consumes draw space in the early layer that a succeeding
158-
// draw could have utilized.
159-
//
160-
// To mitigate this, we allow stencils to continue searching even after finding a
161-
// CompatibleOverlap, but we penalize the remaining search limit by subtracting half
162-
// of kMaxSearchLimit. This heuristic ensures:
163-
// 1) The search typically isn't blocked by the first compatible overlap, unless
164-
// the match was found deep (over half the limit) into the search.
165-
// 2) If two matches are found, the search halts.
166-
//
167-
// Ultimately, this is an imprecise heuristic. In an ideal world, we would maximize
168-
// batching by exhaustively searching to the end of the list, but that would degrade
169-
// insertion performance to O(n^2).
170-
if (overlapType == BoundsTest::kCompatibleOverlap) {
171-
if (isStencil) {
172-
limit -= kMaxSearchLimit >> 1;
173-
} else {
174-
return true;
175-
}
122+
// To support deeper searches while mitigating search time, if we found a matching
123+
// BindingList then we penalize the remaining search limit by subtracting half of
124+
// kMaxSearchLimit. Ultimately this is an imprecise heuristic. In an ideal world, we
125+
// would maximize batching by exhaustively searching to the end of the list, but that
126+
// would degrade insertion performance to O(n^2).
127+
if (match) {
128+
limit -= kMaxSearchLimit >> 1;
129+
targetMatch = match;
176130
}
177-
return false;
131+
} else if (match && canForwardMerge) {
132+
SkASSERT(!allowedBeforeLayer &&
133+
!requiresBarrier &&
134+
current == fLayers.tail());
135+
forwardMerge = match;
178136
}
179-
SkUNREACHABLE;
180-
};
181137

182-
for (; limit >= 0; --limit) {
183-
if (current == stop || processLayer()) {
138+
if (!allowedBeforeLayer || current == stop) {
184139
break;
140+
} else {
141+
current = current->fPrev;
185142
}
186-
current = current->fPrev;
187-
}
188-
if (!targetMatch && current == stop && current) {
189-
processLayer();
190143
}
191144

145+
SkASSERT(!targetLayer || !stop || targetLayer->fOrder >= stop->fOrder);
146+
192147
if (!targetLayer) {
193148
fOrderCounter = fOrderCounter.next();
194149
targetLayer = fStorage.make<Layer>(fOrderCounter);
@@ -198,6 +153,7 @@ std::pair<Layer*, BindingList*> DrawListLayer::searchBackwards(
198153
current->fBindings.remove(forwardMerge);
199154
targetLayer->fBindings.addToHead(forwardMerge);
200155
forwardMerge->fOrder = CompressedPaintersOrder::First();
156+
targetMatch = forwardMerge;
201157
}
202158
fLayers.addToTail(targetLayer);
203159
}

0 commit comments

Comments
 (0)