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

[Impeller] Reduce allocations for polyline generation #47837

Merged
auto-submit merged 15 commits into from
Nov 10, 2023
Merged

[Impeller] Reduce allocations for polyline generation #47837

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1bc0c4d
1
dnfield Nov 8, 2023
ebfaabb
[Impeller] Reduce allocations in polyline generation
dnfield Nov 8, 2023
84d2e20
merge
dnfield Nov 8, 2023
a92b757
fixes for tests, minor cleanup/rename
dnfield Nov 8, 2023
aa280ec
docs
dnfield Nov 8, 2023
cd60b9f
one more reserve
dnfield Nov 8, 2023
07eee60
No TLS, unique_ptr for vector with callback
dnfield Nov 9, 2023
761be29
more review
dnfield Nov 9, 2023
4d9fe4e
merge
dnfield Nov 9, 2023
720a0ee
names
dnfield Nov 9, 2023
615f8fd
tests
dnfield Nov 9, 2023
71afaa0
lint
dnfield Nov 9, 2023
da16afe
lint
dnfield Nov 9, 2023
d9a0ab9
Merge branch 'main' into alloc
dnfield Nov 10, 2023
eaa07be
Merge branch 'main' into alloc
dnfield Nov 10, 2023
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
36 changes: 0 additions & 36 deletions impeller/entity/entity_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2354,42 +2354,6 @@ TEST_P(EntityTest, TiledTextureContentsIsOpaque) {
ASSERT_FALSE(contents.IsOpaque());
}

TEST_P(EntityTest, TessellateConvex) {
{
// Sanity check simple rectangle.
auto [pts, indices] =
TessellateConvex(PathBuilder{}
.AddRect(Rect::MakeLTRB(0, 0, 10, 10))
.TakePath()
.CreatePolyline(1.0));

std::vector<Point> expected = {
{0, 0}, {10, 0}, {10, 10}, {0, 10}, //
};
std::vector<uint16_t> expected_indices = {0, 1, 2, 0, 2, 3};
ASSERT_EQ(pts, expected);
ASSERT_EQ(indices, expected_indices);
}

{
auto [pts, indices] =
TessellateConvex(PathBuilder{}
.AddRect(Rect::MakeLTRB(0, 0, 10, 10))
.AddRect(Rect::MakeLTRB(20, 20, 30, 30))
.TakePath()
.CreatePolyline(1.0));

std::vector<Point> expected = {
{0, 0}, {10, 0}, {10, 10}, {0, 10}, //
{20, 20}, {30, 20}, {30, 30}, {20, 30} //
};
std::vector<uint16_t> expected_indices = {0, 1, 2, 0, 2, 3,
0, 6, 7, 0, 7, 8};
ASSERT_EQ(pts, expected);
ASSERT_EQ(indices, expected_indices);
}
}

TEST_P(EntityTest, PointFieldGeometryDivisions) {
// Square always gives 4 divisions.
ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(24.0, false), 4u);
Expand Down
14 changes: 6 additions & 8 deletions impeller/entity/geometry/fill_path_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ GeometryResult FillPathGeometry::GetPositionBuffer(

if (path_.GetFillType() == FillType::kNonZero && //
path_.IsConvex()) {
auto [points, indices] = TessellateConvex(
path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()));
auto [points, indices] = renderer.GetTessellator()->TessellateConvex(
path_, entity.GetTransformation().GetMaxBasisLength());

vertex_buffer.vertex_buffer = host_buffer.Emplace(
points.data(), points.size() * sizeof(Point), alignof(Point));
Expand All @@ -42,8 +42,7 @@ GeometryResult FillPathGeometry::GetPositionBuffer(
}

auto tesselation_result = renderer.GetTessellator()->Tessellate(
path_.GetFillType(),
path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()),
path_, entity.GetTransformation().GetMaxBasisLength(),
[&vertex_buffer, &host_buffer](
const float* vertices, size_t vertices_count, const uint16_t* indices,
size_t indices_count) {
Expand Down Expand Up @@ -87,8 +86,8 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer(

if (path_.GetFillType() == FillType::kNonZero && //
path_.IsConvex()) {
auto [points, indices] = TessellateConvex(
path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()));
auto [points, indices] = renderer.GetTessellator()->TessellateConvex(
path_, entity.GetTransformation().GetMaxBasisLength());

VertexBufferBuilder<VS::PerVertexData> vertex_builder;
vertex_builder.Reserve(points.size());
Expand All @@ -115,8 +114,7 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer(

VertexBufferBuilder<VS::PerVertexData> vertex_builder;
auto tesselation_result = renderer.GetTessellator()->Tessellate(
path_.GetFillType(),
path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()),
path_, entity.GetTransformation().GetMaxBasisLength(),
[&vertex_builder, &uv_transform](
const float* vertices, size_t vertices_count, const uint16_t* indices,
size_t indices_count) {
Expand Down
31 changes: 0 additions & 31 deletions impeller/entity/geometry/geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,37 +15,6 @@

namespace impeller {

/// Given a convex polyline, create a triangle fan structure.
std::pair<std::vector<Point>, std::vector<uint16_t>> TessellateConvex(
Path::Polyline polyline) {
std::vector<Point> output;
std::vector<uint16_t> indices;

for (auto j = 0u; j < polyline.contours.size(); j++) {
auto [start, end] = polyline.GetContourPointBounds(j);
auto center = polyline.points[start];

// Some polygons will not self close and an additional triangle
// must be inserted, others will self close and we need to avoid
// inserting an extra triangle.
if (polyline.points[end - 1] == polyline.points[start]) {
end--;
}
output.emplace_back(center);
output.emplace_back(polyline.points[start + 1]);

for (auto i = start + 2; i < end; i++) {
const auto& point_b = polyline.points[i];
output.emplace_back(point_b);

indices.emplace_back(0);
indices.emplace_back(i - 1);
indices.emplace_back(i);
}
}
return std::make_pair(output, indices);
}

VertexBufferBuilder<TextureFillVertexShader::PerVertexData>
ComputeUVGeometryCPU(
VertexBufferBuilder<SolidFillVertexShader::PerVertexData>& input,
Expand Down
5 changes: 0 additions & 5 deletions impeller/entity/geometry/geometry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,6 @@ GeometryResult ComputeUVGeometryForRect(Rect source_rect,
const Entity& entity,
RenderPass& pass);

/// @brief Given a polyline created from a convex filled path, perform a
/// tessellation.
std::pair<std::vector<Point>, std::vector<uint16_t>> TessellateConvex(
Path::Polyline polyline);

class Geometry {
public:
Geometry();
Expand Down
14 changes: 9 additions & 5 deletions impeller/entity/geometry/stroke_path_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,9 @@ StrokePathGeometry::JoinProc StrokePathGeometry::GetJoinProc(Join stroke_join) {
PathBuilder::kArcApproximationMagic * alignment *
dir;

auto arc_points = CubicPathComponent(start_offset, start_handle,
middle_handle, middle)
.CreatePolyline(scale);
std::vector<Point> arc_points;
CubicPathComponent(start_offset, start_handle, middle_handle, middle)
Copy link
Member

@gaaclarke gaaclarke Nov 9, 2023

Choose a reason for hiding this comment

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

Can the reserve size be calculated here?

Copy link
Contributor Author

@dnfield dnfield Nov 9, 2023

Choose a reason for hiding this comment

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

These are small and shouldn't result in lots of reallocation. I'd feel more comfortable having a benchmark showing this before trying to place a number in it.

Copy link
Member

@gaaclarke gaaclarke Nov 9, 2023

Choose a reason for hiding this comment

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

I ask because I'm turning on a linter for this in #47868. I don't think it will flag this though since it can't trivially know what the reserve size is.

Copy link
Contributor

@jonahwilliams jonahwilliams Nov 9, 2023

Choose a reason for hiding this comment

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

Reserving incorrect sizes can make things worse. If we don't know let's use the standard vec reallocation startegy

.AppendPolylinePoints(scale, arc_points);

VS::PerVertexData vtx;
for (const auto& point : arc_points) {
Expand Down Expand Up @@ -192,7 +192,9 @@ StrokePathGeometry::CapProc StrokePathGeometry::GetCapProc(Cap stroke_cap) {
vtx_builder.AppendVertex(vtx);
vtx.position = position - orientation;
vtx_builder.AppendVertex(vtx);
for (const auto& point : arc.CreatePolyline(scale)) {
std::vector<Point> arc_points;
Copy link
Member

@gaaclarke gaaclarke Nov 9, 2023

Choose a reason for hiding this comment

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

Or here?

Copy link
Contributor Author

@dnfield dnfield Nov 9, 2023

Choose a reason for hiding this comment

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

Same deal. This shouldn't typically result in a large number of points.

arc.AppendPolylinePoints(scale, arc_points);
for (const auto& point : arc_points) {
vtx.position = position + point;
vtx_builder.AppendVertex(vtx);
vtx.position = position + (-point).Reflect(forward_normal);
Expand Down Expand Up @@ -234,7 +236,9 @@ StrokePathGeometry::CreateSolidStrokeVertices(
const StrokePathGeometry::CapProc& cap_proc,
Scalar scale) {
VertexBufferBuilder<VS::PerVertexData> vtx_builder;
auto polyline = path.CreatePolyline(scale);
std::vector<Point> point_buffer;
point_buffer.reserve(512);
Copy link
Contributor

@jonahwilliams jonahwilliams Nov 8, 2023

Choose a reason for hiding this comment

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

document magic numbers? what is this from?

Copy link
Contributor Author

@dnfield dnfield Nov 9, 2023

Choose a reason for hiding this comment

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

I made it up!

Shall I add a comment saying I made this up?

Copy link
Contributor

@jonahwilliams jonahwilliams Nov 9, 2023

Choose a reason for hiding this comment

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

Yes please!

Copy link
Contributor

@jonahwilliams jonahwilliams Nov 9, 2023

Choose a reason for hiding this comment

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

That way we remember its not a Chesterton's Fence situation in the future.

Copy link
Contributor Author

@dnfield dnfield Nov 9, 2023

Choose a reason for hiding this comment

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

Done

auto polyline = path.CreatePolyline(scale, point_buffer);

VS::PerVertexData vtx;

Expand Down
27 changes: 20 additions & 7 deletions impeller/geometry/geometry_benchmarks.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,28 @@ static void BM_Polyline(benchmark::State& state, Args&&... args) {

size_t point_count = 0u;
size_t single_point_count = 0u;
std::vector<Point> points;
points.reserve(2048);
while (state.KeepRunning()) {
auto polyline = path.CreatePolyline(1.0f);
single_point_count = polyline.points.size();
point_count += single_point_count;
if (tessellate) {
tess.Tessellate(
FillType::kNonZero, polyline,
[](const float* vertices, size_t vertices_count,
const uint16_t* indices, size_t indices_count) { return true; });
tess.Tessellate(path, 1.0f,
[&point_count, &single_point_count](
const float* vertices, size_t vertices_count,
const uint16_t* indices, size_t indices_count) {
if (indices_count > 0) {
single_point_count = indices_count;
point_count += indices_count;
} else {
single_point_count = vertices_count;
point_count += vertices_count;
}
return true;
});
} else {
auto polyline = path.CreatePolyline(1.0f, points);
single_point_count = polyline.points.size();
point_count += single_point_count;
points.clear();
}
}
state.counters["SinglePointCount"] = single_point_count;
Expand Down
21 changes: 14 additions & 7 deletions impeller/geometry/geometry_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,8 @@ TEST(GeometryTest, EmptyPath) {
path.GetContourComponentAtIndex(0, c);
ASSERT_POINT_NEAR(c.destination, Point());

Path::Polyline polyline = path.CreatePolyline(1.0f);
std::vector<Point> polyline_points;
Path::Polyline polyline = path.CreatePolyline(1.0f, polyline_points);
ASSERT_TRUE(polyline.points.empty());
ASSERT_TRUE(polyline.contours.empty());
}
Expand Down Expand Up @@ -2099,7 +2100,8 @@ TEST(GeometryTest, RectRoundOut) {

TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) {
CubicPathComponent component({10, 10}, {20, 35}, {35, 20}, {40, 40});
auto polyline = component.CreatePolyline(1.0f);
std::vector<Point> polyline;
component.AppendPolylinePoints(1.0f, polyline);
ASSERT_NE(polyline.front().x, 10);
ASSERT_NE(polyline.front().y, 10);
ASSERT_EQ(polyline.back().x, 40);
Expand All @@ -2114,7 +2116,8 @@ TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) {
builder.MoveTo({40, 40});
builder.LineTo({50, 50});

auto polyline = builder.TakePath().CreatePolyline(1.0f);
std::vector<Point> points;
auto polyline = builder.TakePath().CreatePolyline(1.0f, points);

ASSERT_EQ(polyline.contours.size(), 2u);
ASSERT_EQ(polyline.points.size(), 5u);
Expand Down Expand Up @@ -2196,14 +2199,15 @@ TEST(GeometryTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) {
}

TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) {
std::vector<Point> polyline_points;
Path::Polyline polyline = PathBuilder{}
.AddLine({100, 100}, {200, 100})
.MoveTo({100, 200})
.LineTo({150, 250})
.LineTo({200, 200})
.Close()
.TakePath()
.CreatePolyline(1.0f);
.CreatePolyline(1.0f, polyline_points);
ASSERT_EQ(polyline.points.size(), 6u);
ASSERT_EQ(polyline.contours.size(), 2u);
ASSERT_EQ(polyline.contours[0].is_closed, false);
Expand All @@ -2213,14 +2217,15 @@ TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) {
}

TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) {
std::vector<Point> polyline_points;
Path::Polyline polyline = PathBuilder{}
.AddLine({100, 100}, {200, 100})
.MoveTo({100, 200})
.LineTo({150, 250})
.LineTo({200, 200})
.Close()
.TakePath()
.CreatePolyline(1.0f);
.CreatePolyline(1.0f, polyline_points);
size_t a1, a2, b1, b2;
std::tie(a1, a2) = polyline.GetContourPointBounds(0);
std::tie(b1, b2) = polyline.GetContourPointBounds(1);
Expand All @@ -2231,10 +2236,11 @@ TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) {
}

TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) {
std::vector<Point> polyline_points;
Path::Polyline polyline = PathBuilder{}
.AddRect(Rect::MakeLTRB(50, 60, 70, 80))
.TakePath()
.CreatePolyline(1.0f);
.CreatePolyline(1.0f, polyline_points);
ASSERT_EQ(polyline.contours.size(), 1u);
ASSERT_TRUE(polyline.contours[0].is_closed);
ASSERT_EQ(polyline.contours[0].start_index, 0u);
Expand All @@ -2247,6 +2253,7 @@ TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) {
}

TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) {
std::vector<Point> polyline_points;
Path::Polyline polyline =
PathBuilder{}
.MoveTo({50, 50})
Expand All @@ -2259,7 +2266,7 @@ TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) {
.LineTo({0, 100})
.LineTo({0, 100}) // Insert duplicate at end of contour.
.TakePath()
.CreatePolyline(1.0f);
.CreatePolyline(1.0f, polyline_points);
ASSERT_EQ(polyline.contours.size(), 2u);
ASSERT_EQ(polyline.contours[0].start_index, 0u);
ASSERT_TRUE(polyline.contours[0].is_closed);
Expand Down
38 changes: 12 additions & 26 deletions impeller/geometry/path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,26 +269,12 @@ bool Path::UpdateContourComponentAtIndex(size_t index,
return true;
}

Path::Polyline Path::CreatePolyline(Scalar scale) const {
Polyline polyline;
Path::Polyline::Polyline(std::vector<Point>& point_buffer)
: points(point_buffer) {}

std::optional<Point> previous_contour_point;
auto collect_points = [&polyline, &previous_contour_point](
const std::vector<Point>& collection) {
if (collection.empty()) {
return;
}

for (const auto& point : collection) {
if (previous_contour_point.has_value() &&
previous_contour_point.value() == point) {
// Skip over duplicate points in the same contour.
continue;
}
previous_contour_point = point;
polyline.points.push_back(point);
}
};
Path::Polyline Path::CreatePolyline(Scalar scale,
std::vector<Point>& point_buffer) const {
Polyline polyline(point_buffer);

auto get_path_component = [this](size_t component_i) -> PathComponentVariant {
if (component_i >= components_.size()) {
Expand Down Expand Up @@ -328,8 +314,8 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const {
std::optional<size_t> previous_path_component_index;
auto end_contour = [&polyline, &previous_path_component_index,
&get_path_component, &components]() {
// Whenever a contour has ended, extract the exact end direction from the
// last component.
// Whenever a contour has ended, extract the exact end direction from
// the last component.
if (polyline.contours.empty()) {
return;
}
Expand Down Expand Up @@ -370,23 +356,23 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const {
.component_start_index = polyline.points.size() - 1,
.is_curve = false,
});
collect_points(linears_[component.index].CreatePolyline());
linears_[component.index].AppendPolylinePoints(polyline.points);
previous_path_component_index = component_i;
break;
case ComponentType::kQuadratic:
components.push_back({
.component_start_index = polyline.points.size() - 1,
.is_curve = true,
});
collect_points(quads_[component.index].CreatePolyline(scale));
quads_[component.index].AppendPolylinePoints(scale, polyline.points);
previous_path_component_index = component_i;
break;
case ComponentType::kCubic:
components.push_back({
.component_start_index = polyline.points.size() - 1,
.is_curve = true,
});
collect_points(cubics_[component.index].CreatePolyline(scale));
cubics_[component.index].AppendPolylinePoints(scale, polyline.points);
previous_path_component_index = component_i;
break;
case ComponentType::kContour:
Expand All @@ -403,8 +389,8 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const {
.is_closed = contour.is_closed,
.start_direction = start_direction,
.components = components});
previous_contour_point = std::nullopt;
collect_points({contour.destination});

polyline.points.push_back(contour.destination);
break;
}
}
Expand Down
Loading