Delete VolatilePathTracker in favor of Dispatch tracking #55125
Conversation
| uint32_t use_count = 0u; | ||
| }; | ||
|
|
||
| std::shared_ptr<Data> data_; |
There was a problem hiding this comment.
This shared_ptr enables global conversion and volatile tracking across all instances. It will collapse with another shared_ptr when we stop serving two backends and settle on a single impl.
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
I took a quick pass over this, will take a closer look tomorrow.
I think this is a nice simplification, though we should long term plan on ripping this out for Impeller (probably after mobile is done, but maybe before desktop ) since we don't know how impactful volatile paths are for Skia.
display_list/dl_canvas.h
Outdated
| ClipOp clip_op = ClipOp::kIntersect, | ||
| bool is_aa = false) { | ||
| ClipPath(path.GetSkPath(), clip_op, is_aa); | ||
| ClipPath(path.GetSkPath(true), clip_op, is_aa); |
There was a problem hiding this comment.
minor nit. For these sorts of booleans, I like the argument comments `/will_render=/true
There was a problem hiding this comment.
Or use a second method with a more obvious name?
There was a problem hiding this comment.
if its just used in a few places the comment is probably fine, but your choice.
display_list/geometry/dl_path.cc
Outdated
|
|
||
| bool DlPath::IsConverted() const { | ||
| if (!path_.IsEmpty()) { | ||
| if (!data_->path.IsEmpty()) { |
There was a problem hiding this comment.
I don't know if we should use something like isEmpty here. if we're given a totally empty path, then we'd keep reconverting it each frame ... though I guess if it is an empty path maybe that doesn't matter.
I think the fact that this method works does mean that we're default initializing the internal impeller::Path shared_ptr, and then overwriting it later with a converted path. Maybe adding the optional would help with that.
There was a problem hiding this comment.
This just echoes the logic in the GetPath method. I think if the sk_path is empty we just leave the impeller::Path at its default.
| uint32_t use_count = 0u; | ||
| }; | ||
|
|
||
| std::shared_ptr<Data> data_; |
There was a problem hiding this comment.
Why is this a shared_ptr if the CanvasPath class stores sk path and impeller_path directly?
There was a problem hiding this comment.
We copy these objects around all over. Everything that needs to be shared between them needs to be encapsulated here in some "shareable" form.
CP stores SkPath directly to edit it. It is copied (invoking copy-on-write for the original SkPath) into the DlPath object when it is retrieved. That DlPath object then is responsible for marking its own copy of SkPath as non-volatile and for creating the iPath as needed.
Until you modify the CanvasPath again, all future retrievers will get a copy of the same DlPath that was first handed out and so they will share the use count and the converted path.
…155194) flutter/engine@ab9daaa...4d8d851 2024-09-14 [email protected] Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` (flutter/engine#55157) 2024-09-13 [email protected] Roll Skia from bdc5e73cb6c9 to 2b8e33aa4824 (1 revision) (flutter/engine#55192) 2024-09-13 [email protected] Roll Dart SDK from 302b6472b849 to c0f7e399ff4a (1 revision) (flutter/engine#55191) 2024-09-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[skwasm] Scene builder optimizations for platform view placement (#54949)" (flutter/engine#55193) 2024-09-13 [email protected] Delete VolatilePathTracker in favor of Dispatch tracking (flutter/engine#55125) 2024-09-13 [email protected] Roll Fuchsia Linux SDK from 3YH1DEYJ-s93fHBw5... to -kKh_AYzPh_iEmTxK... (flutter/engine#55190) 2024-09-13 [email protected] Roll Skia from 9877f459399a to bdc5e73cb6c9 (1 revision) (flutter/engine#55189) 2024-09-13 [email protected] [impeller] add Android flag for disabling surface control for debugging. (flutter/engine#55185) 2024-09-13 [email protected] Roll Skia from a5a6d12b3642 to 9877f459399a (2 revisions) (flutter/engine#55187) 2024-09-13 [email protected] Roll Dart SDK from eb664303c5ff to 302b6472b849 (2 revisions) (flutter/engine#55182) 2024-09-13 [email protected] [skwasm] Scene builder optimizations for platform view placement (flutter/engine#54949) 2024-09-13 [email protected] add back test itSendsTextShowPasswordToFrameworkOnAttach with new mock for display metrics (flutter/engine#55110) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 3YH1DEYJ-s93 to -kKh_AYzPh_i If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
ui.Canvas and ui.SceneBuilder now use the DlPath object directly from the ui.Path object. This results in increased sharing of the wrapper objects which then increases the sharing of both the converted Impeller paths and Skia's volatile flag.
The VolatilePathTracker mechanism is deleted and rather than count the number of frames that a path is stable for, instead we count the number of times it is used for rendering. If a path is used 100 times in a single frame, it will become non-volatile and start being cached almost immediately. The cached Impeller paths are now also tracked for all instances of the same path, rather than for each call site that originated from a DisplayList dispatch.