Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Set SkPath::setIsVolatile based on whether the path survives at least two frames #22620

Merged
dnfield merged 23 commits into from
Dec 10, 2020
Merged

Set SkPath::setIsVolatile based on whether the path survives at least two frames #22620

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
bc78eea
does not work yet
dnfield Nov 19, 2020
f2867c8
safe?
dnfield Nov 21, 2020
34e2523
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Nov 21, 2020
100b2ec
one more
dnfield Nov 21, 2020
6a84f9e
missing mutex
dnfield Nov 21, 2020
24df8c7
UIDartState/shell ownership
dnfield Nov 21, 2020
280b3c9
review and fixup errors introduced
dnfield Nov 30, 2020
07d2d99
test
dnfield Nov 30, 2020
88d4125
benchmark and improvement
dnfield Dec 1, 2020
62c238f
oops
dnfield Dec 1, 2020
9fc3504
..
dnfield Dec 1, 2020
c8171ad
more review
dnfield Dec 1, 2020
17eefed
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 1, 2020
9cd0ac9
deflake tests
dnfield Dec 1, 2020
e087e3d
fix dangling pointer issue
dnfield Dec 1, 2020
50dadd3
missed one
dnfield Dec 1, 2020
38281c2
NO MORE FLAKES
dnfield Dec 2, 2020
7767698
unnecessary code
dnfield Dec 2, 2020
b1d122e
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 2, 2020
847a998
update new test
dnfield Dec 3, 2020
cfaa5b0
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 8, 2020
71153fe
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 10, 2020
1eac95f
review/update tests so I can run them with null safety
dnfield Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ FILE: ../../../flutter/lib/ui/painting/path.cc
FILE: ../../../flutter/lib/ui/painting/path.h
FILE: ../../../flutter/lib/ui/painting/path_measure.cc
FILE: ../../../flutter/lib/ui/painting/path_measure.h
FILE: ../../../flutter/lib/ui/painting/path_unittests.cc
FILE: ../../../flutter/lib/ui/painting/picture.cc
FILE: ../../../flutter/lib/ui/painting/picture.h
FILE: ../../../flutter/lib/ui/painting/picture_recorder.cc
Expand Down Expand Up @@ -404,6 +405,8 @@ FILE: ../../../flutter/lib/ui/ui.dart
FILE: ../../../flutter/lib/ui/ui_benchmarks.cc
FILE: ../../../flutter/lib/ui/ui_dart_state.cc
FILE: ../../../flutter/lib/ui/ui_dart_state.h
FILE: ../../../flutter/lib/ui/volatile_path_tracker.cc
FILE: ../../../flutter/lib/ui/volatile_path_tracker.h
FILE: ../../../flutter/lib/ui/window.dart
FILE: ../../../flutter/lib/ui/window/platform_configuration.cc
FILE: ../../../flutter/lib/ui/window/platform_configuration.h
Expand Down
3 changes: 3 additions & 0 deletions lib/ui/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ source_set("ui") {
"text/text_box.h",
"ui_dart_state.cc",
"ui_dart_state.h",
"volatile_path_tracker.cc",
"volatile_path_tracker.h",
"window/platform_configuration.cc",
"window/platform_configuration.h",
"window/platform_message.cc",
Expand Down Expand Up @@ -188,6 +190,7 @@ if (enable_unittests) {
sources = [
"painting/image_dispose_unittests.cc",
"painting/image_encoding_unittests.cc",
"painting/path_unittests.cc",
"painting/vertices_unittests.cc",
"window/platform_configuration_unittests.cc",
"window/pointer_data_packet_converter_unittests.cc",
Expand Down
27 changes: 26 additions & 1 deletion lib/ui/fixtures/ui_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,25 @@ void createVertices() {
);
_validateVertices(vertices);
}

void _validateVertices(Vertices vertices) native 'ValidateVertices';

@pragma('vm:entry-point')
void createRetainedPath() {
final Path path = Path()..lineTo(10, 10);
_validatePath(path);
// Arbitrarily hold a reference to the path to make sure it does not get
// garbage collected.
Future<void>.delayed(const Duration(days: 100)).then((_) {
path.lineTo(100, 100);
});
}
@pragma('vm:entry-point')
void createCollectablePath() {
final Path path = Path()..lineTo(10, 10);
_validatePath(path);
}
void _validatePath(Path path) native 'ValidatePath';

@pragma('vm:entry-point')
void frameCallback(FrameInfo info) {
print('called back');
Expand Down Expand Up @@ -132,3 +148,12 @@ Future<void> pumpImage() async {
}
void _captureImageAndPicture(Image image, Picture picture) native 'CaptureImageAndPicture';
Future<void> _onBeginFrameDone() native 'OnBeginFrameDone';

@pragma('vm:entry-point')
void create100CollectablePaths() {

@chinmaygarde chinmaygarde Dec 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. This seems dead code. Did you forget to check in the test maybe?

@dnfield dnfield Dec 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to remove this code when I changed how a test worked.

const int numberOfPaths = 100;
final List<Path> paths = List<Path>.generate(numberOfPaths, (int index) {
return Path()..lineTo(index.toDouble(), index.toDouble() * 2);
});
assert(paths.length == numberOfPaths);
}
134 changes: 93 additions & 41 deletions lib/ui/painting/path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,43 +67,71 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) {
FOR_EACH_BINDING(DART_REGISTER_NATIVE)});
}

CanvasPath::CanvasPath() {}
CanvasPath::CanvasPath()
: path_tracker_(UIDartState::Current()->GetVolatilePathTracker()),
tracked_path_(std::make_shared<VolatilePathTracker::Path>()) {
FML_DCHECK(path_tracker_);
resetVolatility();
}

CanvasPath::~CanvasPath() = default;

CanvasPath::~CanvasPath() {}
void CanvasPath::resetVolatility() {

@liyuqian liyuqian Nov 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name resetVolatility sounds like it will also reset the volatility frame count but that does not seem to be the case. I believe resetting the count is the intended behavior as it's called in those methods that mutate the path.

@dnfield dnfield Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (!tracked_path_->tracking_volatility) {
mutable_path().setIsVolatile(true);
tracked_path_->frame_count = 0;

@chinmaygarde chinmaygarde Dec 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems redundant.

@dnfield dnfield Dec 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this path had previously been marked as non-volatile, but now has been mutated and is volatile again, we have to reset the frame count so that we don't immediately mark it volatile if it is only drawn for e.g. 1 frame.

tracked_path_->tracking_volatility = true;
path_tracker_->Insert(tracked_path_);
}
}

void CanvasPath::ReleaseDartWrappableReference() const {
FML_DCHECK(path_tracker_);
if (tracked_path_->tracking_volatility) {

@chinmaygarde chinmaygarde Dec 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can happen on any thread right? For example, the thread on which the GC happens. In that case, isn't there a data race between this and the the setter in resetVolatility? Maybe you are depending on the fact that once the Dart VM releases the reference to the path, we won't be in a situation where we will call reset volatility. But I am not sure that we can guarantee that case since we could still be holding onto the shared pointer on the native side.

@chinmaygarde chinmaygarde Dec 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tracking volatility from the get go in the constructor and not needing the flag (and its unsafe check) should make it so that you don't need the check.

@jason-simmons jason-simmons Dec 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would be safe if the tracking_volatility flag was made an atomic<bool>.

This relies on the contract that tracking_volatility will only be set to true by resetVolatility, which only happens during Dart->native method calls that run on the UI thread. When ReleaseDartWrappableReference is called, no such method calls can be active because the Dart object is being GCed.

There is a race against VolatilePathTracker::OnFrame which could be running concurrently on the UI thread and might clear tracking_volatility while removing the path from the tracker. However, the worst case is that ReleaseDartWrappableReference does an unnecessaryErase of a path that was already removed. If that happens Erase/Drain will just call unordered_set::erase for that path which will do nothing.

However, this is confusing and makes it easy to introduce errors.

The advantage of the check is avoiding the performance cost of the Erase (which might take a lock) and the later Drain if the GCed path is not tracked.

If the cost is small, another alternative would be to always call Erase in ReleaseDartWrappableReference and avoid reading tracking_volatility on the GC thread. Drain on the UI thread could then safely check tracking_volatility in order to avoid doing the unordered_set::erase if the path is not tracked.

@dnfield dnfield Dec 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my thought is that at best, this will save us taking a lock, and at worst we'll get a data race that causes us to insert this into the collection even though the path has been GC'd, and it will be removed anyway.

I guess the risk is that some refactor happens which changes that worst case scenario to something bad.

@chinmaygarde - WDYT about making it an atomic<bool> and documenting this expectation?

@chinmaygarde chinmaygarde Dec 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no such method calls can be active because the Dart object is being GCed.

I was thinking of calls to CanvasPath::Create from the native side. But I suppose that doesn't happen today. I may be overthinking cases where these objects are created from the native side.

If the cost is small, another alternative would be to always call Erase in ReleaseDartWrappableReference and avoid reading tracking_volatility on the GC thread.

IMO, that would be a lot clearer.

@dnfield dnfield Dec 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG, done. We can always come back to this later with a benchmark if it makes a difference.

path_tracker_->Erase(tracked_path_);
}
}

int CanvasPath::getFillType() {
return static_cast<int>(path_.getFillType());
return static_cast<int>(path().getFillType());
}

void CanvasPath::setFillType(int fill_type) {
path_.setFillType(static_cast<SkPathFillType>(fill_type));
mutable_path().setFillType(static_cast<SkPathFillType>(fill_type));
resetVolatility();
}

void CanvasPath::moveTo(float x, float y) {
path_.moveTo(x, y);
mutable_path().moveTo(x, y);
resetVolatility();
}

void CanvasPath::relativeMoveTo(float x, float y) {
path_.rMoveTo(x, y);
mutable_path().rMoveTo(x, y);
resetVolatility();
}

void CanvasPath::lineTo(float x, float y) {
path_.lineTo(x, y);
mutable_path().lineTo(x, y);
resetVolatility();
}

void CanvasPath::relativeLineTo(float x, float y) {
path_.rLineTo(x, y);
mutable_path().rLineTo(x, y);
resetVolatility();
}

void CanvasPath::quadraticBezierTo(float x1, float y1, float x2, float y2) {
path_.quadTo(x1, y1, x2, y2);
mutable_path().quadTo(x1, y1, x2, y2);
resetVolatility();
}

void CanvasPath::relativeQuadraticBezierTo(float x1,
float y1,
float x2,
float y2) {
path_.rQuadTo(x1, y1, x2, y2);
mutable_path().rQuadTo(x1, y1, x2, y2);
resetVolatility();
}

void CanvasPath::cubicTo(float x1,
Expand All @@ -112,7 +140,8 @@ void CanvasPath::cubicTo(float x1,
float y2,
float x3,
float y3) {
path_.cubicTo(x1, y1, x2, y2, x3, y3);
mutable_path().cubicTo(x1, y1, x2, y2, x3, y3);
resetVolatility();
}

void CanvasPath::relativeCubicTo(float x1,
Expand All @@ -121,19 +150,22 @@ void CanvasPath::relativeCubicTo(float x1,
float y2,
float x3,
float y3) {
path_.rCubicTo(x1, y1, x2, y2, x3, y3);
mutable_path().rCubicTo(x1, y1, x2, y2, x3, y3);
resetVolatility();
}

void CanvasPath::conicTo(float x1, float y1, float x2, float y2, float w) {
path_.conicTo(x1, y1, x2, y2, w);
mutable_path().conicTo(x1, y1, x2, y2, w);
resetVolatility();
}

void CanvasPath::relativeConicTo(float x1,
float y1,
float x2,
float y2,
float w) {
path_.rConicTo(x1, y1, x2, y2, w);
mutable_path().rConicTo(x1, y1, x2, y2, w);
resetVolatility();
}

void CanvasPath::arcTo(float left,
Expand All @@ -143,9 +175,10 @@ void CanvasPath::arcTo(float left,
float startAngle,
float sweepAngle,
bool forceMoveTo) {
path_.arcTo(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI,
forceMoveTo);
mutable_path().arcTo(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI,
forceMoveTo);
resetVolatility();
}

void CanvasPath::arcToPoint(float arcEndX,
Expand All @@ -160,8 +193,9 @@ void CanvasPath::arcToPoint(float arcEndX,
const auto direction =
isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW;

path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, arcEndX,
arcEndY);
mutable_path().arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction,
arcEndX, arcEndY);
resetVolatility();
}

void CanvasPath::relativeArcToPoint(float arcEndDeltaX,
Expand All @@ -175,16 +209,19 @@ void CanvasPath::relativeArcToPoint(float arcEndDeltaX,
: SkPath::ArcSize::kSmall_ArcSize;
const auto direction =
isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW;
path_.rArcTo(radiusX, radiusY, xAxisRotation, arcSize, direction,
arcEndDeltaX, arcEndDeltaY);
mutable_path().rArcTo(radiusX, radiusY, xAxisRotation, arcSize, direction,
arcEndDeltaX, arcEndDeltaY);
resetVolatility();
}

void CanvasPath::addRect(float left, float top, float right, float bottom) {
path_.addRect(SkRect::MakeLTRB(left, top, right, bottom));
mutable_path().addRect(SkRect::MakeLTRB(left, top, right, bottom));
resetVolatility();
}

void CanvasPath::addOval(float left, float top, float right, float bottom) {
path_.addOval(SkRect::MakeLTRB(left, top, right, bottom));
mutable_path().addOval(SkRect::MakeLTRB(left, top, right, bottom));
resetVolatility();
}

void CanvasPath::addArc(float left,
Expand All @@ -193,25 +230,29 @@ void CanvasPath::addArc(float left,
float bottom,
float startAngle,
float sweepAngle) {
path_.addArc(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI);
mutable_path().addArc(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI);
resetVolatility();
}

void CanvasPath::addPolygon(const tonic::Float32List& points, bool close) {
path_.addPoly(reinterpret_cast<const SkPoint*>(points.data()),
points.num_elements() / 2, close);
mutable_path().addPoly(reinterpret_cast<const SkPoint*>(points.data()),
points.num_elements() / 2, close);
resetVolatility();
}

void CanvasPath::addRRect(const RRect& rrect) {
path_.addRRect(rrect.sk_rrect);
mutable_path().addRRect(rrect.sk_rrect);
resetVolatility();
}

void CanvasPath::addPath(CanvasPath* path, double dx, double dy) {
if (!path) {
Dart_ThrowException(ToDart("Path.addPath called with non-genuine Path."));
return;
}
path_.addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode);
mutable_path().addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode);
resetVolatility();
}

void CanvasPath::addPathWithMatrix(CanvasPath* path,
Expand All @@ -227,8 +268,9 @@ void CanvasPath::addPathWithMatrix(CanvasPath* path,
SkMatrix matrix = ToSkMatrix(matrix4);
matrix.setTranslateX(matrix.getTranslateX() + dx);
matrix.setTranslateY(matrix.getTranslateY() + dy);
path_.addPath(path->path(), matrix, SkPath::kAppend_AddPathMode);
mutable_path().addPath(path->path(), matrix, SkPath::kAppend_AddPathMode);
matrix4.Release();
resetVolatility();
}

void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) {
Expand All @@ -237,7 +279,8 @@ void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) {
ToDart("Path.extendWithPath called with non-genuine Path."));
return;
}
path_.addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode);
mutable_path().addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode);
resetVolatility();
}

void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
Expand All @@ -253,37 +296,45 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
SkMatrix matrix = ToSkMatrix(matrix4);
matrix.setTranslateX(matrix.getTranslateX() + dx);
matrix.setTranslateY(matrix.getTranslateY() + dy);
path_.addPath(path->path(), matrix, SkPath::kExtend_AddPathMode);
mutable_path().addPath(path->path(), matrix, SkPath::kExtend_AddPathMode);
matrix4.Release();
resetVolatility();
}

void CanvasPath::close() {
path_.close();
mutable_path().close();
resetVolatility();
}

void CanvasPath::reset() {
path_.reset();
mutable_path().reset();
resetVolatility();
}

bool CanvasPath::contains(double x, double y) {
return path_.contains(x, y);
return path().contains(x, y);
}

void CanvasPath::shift(Dart_Handle path_handle, double dx, double dy) {
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
path_.offset(dx, dy, &path->path_);
auto other_mutable_path = path->mutable_path();

@jason-simmons jason-simmons Dec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be auto&

@dnfield dnfield Dec 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right - adding a test to cover this

mutable_path().offset(dx, dy, &other_mutable_path);
resetVolatility();
}

void CanvasPath::transform(Dart_Handle path_handle,
tonic::Float64List& matrix4) {
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
path_.transform(ToSkMatrix(matrix4), &path->path_);
auto& other_mutable_path = path->mutable_path();
mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path);
matrix4.Release();
}

tonic::Float32List CanvasPath::getBounds() {
tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4));
const SkRect& bounds = path_.getBounds();
const SkRect& bounds = path().getBounds();
FML_DLOG(ERROR) << "Bounds " << bounds.left() << " " << bounds.top() << " "
<< bounds.right() << " " << bounds.bottom();

@dnfield dnfield Dec 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this. Whups.

rect[0] = bounds.left();
rect[1] = bounds.top();
rect[2] = bounds.right();
Expand All @@ -293,21 +344,22 @@ tonic::Float32List CanvasPath::getBounds() {

bool CanvasPath::op(CanvasPath* path1, CanvasPath* path2, int operation) {
return Op(path1->path(), path2->path(), static_cast<SkPathOp>(operation),
&path_);
&tracked_path_->path);
resetVolatility();
}

void CanvasPath::clone(Dart_Handle path_handle) {
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
// per Skia docs, this will create a fast copy
// data is shared until the source path or dest path are mutated
path->path_ = path_;
path->mutable_path() = this->path();
}

// This is doomed to be called too early, since Paths are mutable.
// However, it can help for some of the clone/shift/transform type methods
// where the resultant path will initially have a meaningful size.
size_t CanvasPath::GetAllocationSize() const {
return sizeof(CanvasPath) + path_.approximateBytesUsed();
return sizeof(CanvasPath) + path().approximateBytesUsed();
}

} // namespace flutter
Loading