The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3d234f50f3d487796c1c859ca7d5636e2932d91

commit c3d234f50f3d487796c1c859ca7d5636e2932d91
Author: danakj <danakj@chromium.org>
Date: Tue Mar 07 18:27:11 2017

Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2738493002
Cr-Commit-Position: refs/heads/master@{#455145}

[modify] https://crrev.com/c3d234f50f3d487796c1c859ca7d5636e2932d91/third_party/WebKit/Source/core/paint/BUILD.gn
[modify] https://crrev.com/c3d234f50f3d487796c1c859ca7d5636e2932d91/third_party/WebKit/Source/core/paint/ClipPathClipper.cpp
[modify] https://crrev.com/c3d234f50f3d487796c1c859ca7d5636e2932d91/third_party/WebKit/Source/core/paint/ClipPathClipper.h
[delete] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/third_party/WebKit/Source/core/paint/SVGClipPainter.cpp
[delete] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/third_party/WebKit/Source/core/paint/SVGClipPainter.h
[modify] https://crrev.com/c3d234f50f3d487796c1c859ca7d5636e2932d91/third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp
[modify] https://crrev.com/c3d234f50f3d487796c1c859ca7d5636e2932d91/third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.h

Recording this here as first step can be merging PaintRecord and cc::DisplayItemList:

danakj [3:38 PM] 
hey so i am thinking about merging the cc::DisplayItemList and cc::PaintRecord types (edited)

[3:38]  
so that we have only a single namespace of operations, instead of like PaintCanvas::translate vs TransformDrawingItem

[3:39]  
I'm thinking that we can do this in a way that doesn't break the current blink::DisplayItemList and cache performance properties as roughly this

[3:39]  
struct Thing { vector<PaintRecord>; } struct PaintRecord { vector<PaintOp> }; union PaintOp { PaintRecord*; transform; drawline; drawchar; drawcolor; etc }

[3:39]  
and a blink::DrawingDisplayItem could just add its paintrecord to Thing's vector, while appending a PaintRecord* to the vector.front()

[3:39]  
a blink::OtherDisplayItem would just append some PaintOps to the Thing.vector.front()

[3:40]  
i wanted to know if yall see any glaring holes in this

[3:40]  
@chrishtr @pdr @wkorman @trchen

[3:42]  
I think for spv2 the Thing struct grows a vector<PaintChunks> that knows about property trees and covers some range of the PaintRecords

Walter Korman [3:44 PM] 
seems interesting to me, i support trying it out

[3:45]  
nothing obvious imo but i have not been following as closely as some the details i think you, enne, chrishtr are working on daily right now

Tien-Ren Chen [3:51 PM] 
Let me try to rephrase. Do you mean PaintRecord will be roughly equivalent of PaintChunk, while Thing will be the equivalent of PaintArtifact?

danakj [4:03 PM] 
Think is roughly equivalent to DisplayItemList but it only has a top level PaintRecord that refers to other PaintRecords as needed

[4:03]  
DrawingDisplayItem would append its PaintRecord to the list and refer to it from an op in the top level PaintRecord instead of from some separate "display item" type

chrishtr [4:04 PM] 
Then Thing == DisplayItemList?

danakj [4:04 PM] 
PaintChunk will still be separate, and refer to paint ops inside the PaintRecords, it would be living right on DisplayItemList roughly, i think

[4:04]  
yah but maybe rename to be less confusing

[4:04]  
cuz its not DisplayItems

[4:04]  
and cuz history

chrishtr [4:04 PM] 
sure

[4:06]  
When you say "add its paintrecord to Thing's vector, while appending a PaintRecord* to the vector.front()" do you mean basically closing off the current PaintRecord, adding the one for the drawing, then starting another one for further future ops?

danakj [4:07 PM] 
no i mean the first PaintRecord can have paintops that are basically a PaintRecord*

[4:07]  
we'd walk that PaintRecord at that time

[4:07]  
then go back to the first list

[4:07]  
er, frst PaintRecord

[4:07]  
youc ould think of it this way also struct Thing { PaintRecord base; vector<PaintRecord> refs }

[4:08]  
a DrawingDisplayItem would add an op to |base| and stick its whole PaintRecord into |refs|

[4:08]  
the op in |base| would refer to the PaintRecord in |refs|

[4:08]  
(i am thinking it would look more like this in practice so vector.front() isn't something special)

[4:10]  
(also worth pointing out if the PaintRecord was small, the impl could choose to just append it directly to |base| instead of putting a pointer to something in |refs|) (edited)

chrishtr [4:12 PM] 
I see.

[4:12]  
And refs would contain only the drawings?

[4:12]  
other ops wouldn't need a ref

danakj [4:14 PM] 
Im not sure exactly what you mean by the drawings

[4:14]  
the refs vector would have basically the insides of DrawingDisplayItems

[4:14]  
and the |base| would have a set of ops, some of which are "do all the ops in this other PaintRecord"

[4:15]  
it makes the recursion be PaintRecord -> PaintRecord instead of DisplayItemList->PaintRecord(->DisplayItemList)

chrishtr [4:15 PM] 
By drawings I meant DrawingDisplayItems

danakj [4:15 PM] 
ah ok, ya, but the insides which is the sk_sp<PaintRecord>

[4:15]  
the cc::DisplayItem type would be no more they would each become some set of ops

[4:16]  
whch they do dynamically when they raster right now

[4:16]  
(well when they raster into a picture)

enne [4:17 PM] 
How do PaintChunks and SimpleLayer (?) relate to blink DisplayItemLists?

chrishtr [4:17 PM] 
@danakj Ah ok. Makes sense.

[4:18]  
@enne  In the current implementation, a PaintChunk refers to a subsequence of a DisplayItemList that has a consistent PropertyTreeState. A PaintChunk and a SimpleLayer are the same thing (SimpleLayer doesn't exist in the code, just design docs)

enne [4:20 PM] 
Ok, then it does sound like blink DisplayItemList / cc DisplayItemList / PaintRecord could all collapse and PaintChunk could then continue to refer to a subsequence of ops in this new thing

chrishtr [4:22 PM] 
Yes, I think so. To echo back: struct PaintArtifact { vector<PaintChunks> paintChunks; Thing thing} struct PaintChunk { pair<int start, int end> endpoints; PropertyTreeState propertyTreeState } . This means that the "top-level" PaintRecord, into which the |endpoints| index, is the onything that has a PropertyTreeState concept

[4:23]  
vector<PaintChunk> that is.

enne [4:24 PM] 
How does this interact with paint caching?

[4:24]  
Since right now that's at the DrawingDisplayitem / PaintRecord level, yeah?

chrishtr [4:26 PM] 
The |ref| would have to store a DisplayItemClient as the cache key (or via another external vector). the Thing object is both the display list and the cache, and so a new Thing would copy PaintRecords from an old Thing via the DisplayItemClient cache.

danakj [4:26 PM] 
@chrishtr  cool, and we could make Thing and PaintArtifact the same thing, making the PaintRecords and PaintChunks direct siblings

chrishtr [4:27 PM] 
Subsequence caching would be accomplished by recording side data structures similar to PaintChunk (ie.. recording beginning and end index) that reference subsequences which are good ideas to cache.

danakj [4:28 PM] 
im still hoping we can get rid of the client concept altogether but we don't need to worry about that for the initial cc change im proposing

chrishtr [4:28 PM] 
(and the subsequence cache also needs to be with a DisplayItemClient key)

[4:28]  
@danakj direct siblings sounds legit

enne [4:29 PM] 
I see, so you'd just copy a subsequence for caching instead of copying a drawing display item

chrishtr [4:30 PM] 
yes. and the client needs to decide whether the subsequence is invalidated

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Where are we at on this?

I hope we can get to this soon. In particular, this will require cleaning up PaintController which has grown very complicated as performance issues were found. Hopefully this year?