[Impeller] Remove impeller::Path copy constructor.#48616
[Impeller] Remove impeller::Path copy constructor.#48616auto-submit[bot] merged 10 commits intoflutter:mainfrom
Conversation
|
I'm not sure if this is reasonable. but since path objects store their point data in a std::vector copying them can be quite expensive. An alternative design would shift path to something similar to the SkPath, where the path data is internally ref counted. |
jonahwilliams
changed the title
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
This lgtm. I think a std::unique_ptr would be easier to use since we wouldn't have to maintain the Clone method. If we can use a private copy constructor somehow that becomes less of an issue.
| Path Path::Clone() const { | ||
| Path new_path; | ||
| new_path.components_ = components_; | ||
| new_path.contours_ = contours_; | ||
| new_path.computed_bounds_ = computed_bounds_; | ||
| new_path.convexity_ = convexity_; | ||
| new_path.points_ = new_path.points_; | ||
| new_path.fill_ = fill_; | ||
| return new_path; | ||
| } |
There was a problem hiding this comment.
Can we make the copy constructor private and call that here? Then we don't have to worry about this atrophying.
impeller/aiks/canvas_recorder.h
Outdated
| return ExecuteAndSerialize(FLT_CANVAS_RECORDER_OP_ARG(DrawPath), path, | ||
| paint); | ||
| void DrawPath(Path path, const Paint& paint) { | ||
| serializer_.Write(path.Clone()); |
There was a problem hiding this comment.
This takes in a reference, no need to clone.
impeller/aiks/canvas_recorder.h
Outdated
| Entity::ClipOperation clip_op = Entity::ClipOperation::kIntersect) { | ||
| return ExecuteAndSerialize(FLT_CANVAS_RECORDER_OP_ARG(ClipPath), path, | ||
| clip_op); | ||
| serializer_.Write(path.Clone()); |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM assuming the benchmarks are improved. I think this is a good model for other classes that are eating up time being copied but we don't want them on the heap. Bonus points if we can get a linter to make sure we are moving when we can.
| .SetConvexity(Convexity::kConvex) | ||
| .TakePath(); | ||
| canvas_.DrawPath(path, paint_); | ||
| canvas_.DrawPath(std::move(path), paint_); |
There was a problem hiding this comment.
I'm guessing the linter isn't helping us here if we miss the std::move too?
There was a problem hiding this comment.
If we don't move its actually a compile error!
jonahwilliams
added
the
autosubmit
flutter/engine@de0ba84...f43ef6c 2023-12-05 skia-flutter-autoroll@skia.org Roll Dart SDK from a1b67665b3a3 to 9c74645153ca (1 revision) (flutter/engine#48648) 2023-12-05 jonahwilliams@google.com [Impeller] Remove impeller::Path copy constructor. (flutter/engine#48616) 2023-12-04 jonahwilliams@google.com [Impeller] Move BufferView/Texture/Sampler when binding. (flutter/engine#48628) 2023-12-04 skia-flutter-autoroll@skia.org Roll Skia from cbd2cf40d63b to d37625f80ac0 (1 revision) (flutter/engine#48643) 2023-12-04 jiahaog@users.noreply.github.com Add `flutter` prefix to import (flutter/engine#48617) 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 matanl@google.com,rmistry@google.com,zra@google.com 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
From looking at profiles, we're always copying paths at least once when recording commands. By deleting the copy constructor, I cna ensure that we're always either moving or explicitly cloning the Path object.
Or, now that I fixed all the moves I could add the copy constructor back.