Skip to content

Commit

Permalink
Improve getting non-overlapping rectangles from RTree (#42399)
Browse files Browse the repository at this point in the history
Fixes flutter/flutter#116070
Fixes flutter/flutter#126202

Introduces `DlRegion` class which implements subset of `SkRegion`
required to get non-overlapping rectangles from region.

The implementation is different and faster than `SkRegion` for this
particular use-case (`display_list_region_benchmarks`):

Edit: Updated benchmark to latest revision and natively (initial run
went through rosetta)
```
----------------------------------------------------------------------------
Benchmark                                  Time             CPU   Iterations
----------------------------------------------------------------------------
BM_RegionBenchmarkDlRegion/Tiny          616 us          616 us          908
BM_RegionBenchmarkSkRegion/Tiny        70559 us        70557 us           10
BM_RegionBenchmarkDlRegion/Small        1315 us         1314 us          537
BM_RegionBenchmarkSkRegion/Small      121736 us       121717 us            6
BM_RegionBenchmarkDlRegion/Medium       1079 us         1079 us          650
BM_RegionBenchmarkSkRegion/Medium      22039 us        22035 us           32
BM_RegionBenchmarkDlRegion/Large         399 us          399 us         1763
BM_RegionBenchmarkSkRegion/Large        1510 us         1510 us          466
```

## Pre-launch Checklist

- [X] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [X] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [X] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [X] I listed at least one issue that this PR fixes in the description
above.
- [X] I added new tests to check the change I am making or feature I am
adding, or Hixie said the PR is test-exempt. See [testing the engine]
for instructions on writing and running engine tests.
- [X] I updated/added relevant documentation (doc comments with `///`).
- [X] I signed the [CLA].
- [X] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
  • Loading branch information
knopp committed Jun 5, 2023
1 parent 7f12e34 commit 042ebae
Show file tree
Hide file tree
Showing 14 changed files with 693 additions and 99 deletions.
1 change: 1 addition & 0 deletions BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ group("flutter") {
public_deps += [
"//flutter/display_list:display_list_benchmarks",
"//flutter/display_list:display_list_builder_benchmarks",
"//flutter/display_list:display_list_region_benchmarks",
"//flutter/fml:fml_benchmarks",
"//flutter/impeller/geometry:geometry_benchmarks",
"//flutter/lib/ui:ui_benchmarks",
Expand Down
1 change: 1 addition & 0 deletions ci/licenses_golden/excluded_files
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
../../../flutter/display_list/effects/dl_image_filter_unittests.cc
../../../flutter/display_list/effects/dl_mask_filter_unittests.cc
../../../flutter/display_list/effects/dl_path_effect_unittests.cc
../../../flutter/display_list/geometry/dl_region_unittests.cc
../../../flutter/display_list/geometry/dl_rtree_unittests.cc
../../../flutter/display_list/skia/dl_sk_conversions_unittests.cc
../../../flutter/display_list/skia/dl_sk_paint_dispatcher_unittests.cc
Expand Down
6 changes: 6 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,7 @@ ORIGIN: ../../../flutter/display_list/benchmarking/dl_complexity_gl.h + ../../..
ORIGIN: ../../../flutter/display_list/benchmarking/dl_complexity_helper.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/benchmarking/dl_complexity_metal.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/benchmarking/dl_complexity_metal.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/benchmarking/dl_region_benchmarks.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/display_list.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/display_list.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_attributes.h + ../../../flutter/LICENSE
Expand Down Expand Up @@ -748,6 +749,8 @@ ORIGIN: ../../../flutter/display_list/effects/dl_path_effect.cc + ../../../flutt
ORIGIN: ../../../flutter/display_list/effects/dl_path_effect.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/effects/dl_runtime_effect.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/effects/dl_runtime_effect.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/geometry/dl_region.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/geometry/dl_region.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/geometry/dl_rtree.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/geometry/dl_rtree.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/image/dl_image.cc + ../../../flutter/LICENSE
Expand Down Expand Up @@ -3380,6 +3383,7 @@ FILE: ../../../flutter/display_list/benchmarking/dl_complexity_gl.h
FILE: ../../../flutter/display_list/benchmarking/dl_complexity_helper.h
FILE: ../../../flutter/display_list/benchmarking/dl_complexity_metal.cc
FILE: ../../../flutter/display_list/benchmarking/dl_complexity_metal.h
FILE: ../../../flutter/display_list/benchmarking/dl_region_benchmarks.cc
FILE: ../../../flutter/display_list/display_list.cc
FILE: ../../../flutter/display_list/display_list.h
FILE: ../../../flutter/display_list/dl_attributes.h
Expand Down Expand Up @@ -3414,6 +3418,8 @@ FILE: ../../../flutter/display_list/effects/dl_path_effect.cc
FILE: ../../../flutter/display_list/effects/dl_path_effect.h
FILE: ../../../flutter/display_list/effects/dl_runtime_effect.cc
FILE: ../../../flutter/display_list/effects/dl_runtime_effect.h
FILE: ../../../flutter/display_list/geometry/dl_region.cc
FILE: ../../../flutter/display_list/geometry/dl_region.h
FILE: ../../../flutter/display_list/geometry/dl_rtree.cc
FILE: ../../../flutter/display_list/geometry/dl_rtree.h
FILE: ../../../flutter/display_list/image/dl_image.cc
Expand Down
15 changes: 15 additions & 0 deletions display_list/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ source_set("display_list") {
"effects/dl_path_effect.h",
"effects/dl_runtime_effect.cc",
"effects/dl_runtime_effect.h",
"geometry/dl_region.cc",
"geometry/dl_region.h",
"geometry/dl_rtree.cc",
"geometry/dl_rtree.h",
"image/dl_image.cc",
Expand Down Expand Up @@ -112,6 +114,7 @@ if (enable_unittests) {
"effects/dl_image_filter_unittests.cc",
"effects/dl_mask_filter_unittests.cc",
"effects/dl_path_effect_unittests.cc",
"geometry/dl_region_unittests.cc",
"geometry/dl_rtree_unittests.cc",
"skia/dl_sk_conversions_unittests.cc",
"skia/dl_sk_paint_dispatcher_unittests.cc",
Expand Down Expand Up @@ -182,6 +185,18 @@ if (enable_unittests) {
"//flutter/testing:testing_lib",
]
}

executable("display_list_region_benchmarks") {
testonly = true

sources = [ "benchmarking/dl_region_benchmarks.cc" ]

deps = [
":display_list_fixtures",
"//flutter/benchmarking",
"//flutter/testing:testing_lib",
]
}
}

fixtures_location("display_list_benchmarks_fixtures") {
Expand Down
92 changes: 92 additions & 0 deletions display_list/benchmarking/dl_region_benchmarks.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/benchmarking/benchmarking.h"

#include "flutter/display_list/geometry/dl_region.h"
#include "third_party/skia/include/core/SkRegion.h"

#include <random>

class SkRegionAdapter {
public:
void addRect(const SkIRect& rect) { region_.op(rect, SkRegion::kUnion_Op); }

std::vector<SkIRect> getRects() {
std::vector<SkIRect> rects;
SkRegion::Iterator it(region_);
while (!it.done()) {
rects.push_back(it.rect());
it.next();
}
return rects;
}

private:
SkRegion region_;
};

class DlRegionAdapter {
public:
void addRect(const SkIRect& rect) { rects_.push_back(rect); }

std::vector<SkIRect> getRects() {
flutter::DlRegion region(std::move(rects_));
return region.getRects(false);
}

private:
std::vector<SkIRect> rects_;
};

template <typename Region>
void RunRegionBenchmark(benchmark::State& state, int maxSize) {
while (state.KeepRunning()) {
std::random_device d;
std::seed_seq seed{2, 1, 3};
std::mt19937 rng(seed);

std::uniform_int_distribution pos(0, 4000);
std::uniform_int_distribution size(1, maxSize);

Region region;

for (int i = 0; i < 2000; ++i) {
SkIRect rect =
SkIRect::MakeXYWH(pos(rng), pos(rng), size(rng), size(rng));
region.addRect(rect);
}

auto vec2 = region.getRects();
}
}

namespace flutter {

static void BM_RegionBenchmarkSkRegion(benchmark::State& state, int maxSize) {
RunRegionBenchmark<SkRegionAdapter>(state, maxSize);
}

static void BM_RegionBenchmarkDlRegion(benchmark::State& state, int maxSize) {
RunRegionBenchmark<DlRegionAdapter>(state, maxSize);
}

BENCHMARK_CAPTURE(BM_RegionBenchmarkDlRegion, Tiny, 30)
->Unit(benchmark::kMicrosecond);
BENCHMARK_CAPTURE(BM_RegionBenchmarkSkRegion, Tiny, 30)
->Unit(benchmark::kMicrosecond);
BENCHMARK_CAPTURE(BM_RegionBenchmarkDlRegion, Small, 100)
->Unit(benchmark::kMicrosecond);
BENCHMARK_CAPTURE(BM_RegionBenchmarkSkRegion, Small, 100)
->Unit(benchmark::kMicrosecond);
BENCHMARK_CAPTURE(BM_RegionBenchmarkDlRegion, Medium, 400)
->Unit(benchmark::kMicrosecond);
BENCHMARK_CAPTURE(BM_RegionBenchmarkSkRegion, Medium, 400)
->Unit(benchmark::kMicrosecond);
BENCHMARK_CAPTURE(BM_RegionBenchmarkDlRegion, Large, 1500)
->Unit(benchmark::kMicrosecond);
BENCHMARK_CAPTURE(BM_RegionBenchmarkSkRegion, Large, 1500)
->Unit(benchmark::kMicrosecond);

} // namespace flutter
3 changes: 2 additions & 1 deletion display_list/dl_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1106,7 +1106,8 @@ void DisplayListBuilder::DrawDisplayList(const sk_sp<DisplayList> display_list,
case BoundsAccumulatorType::kRTree:
auto rtree = display_list->rtree();
if (rtree) {
std::list<SkRect> rects = rtree->searchAndConsolidateRects(bounds);
std::list<SkRect> rects =
rtree->searchAndConsolidateRects(bounds, false);
for (const SkRect& rect : rects) {
// TODO (https://github.com/flutter/flutter/issues/114919): Attributes
// are not necessarily `kDrawDisplayListFlags`.
Expand Down

0 comments on commit 042ebae

Please sign in to comment.