Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Avoid geometry collections copying #15201

Merged
merged 4 commits into from
Jul 24, 2019

Conversation

pozdnyakov
Copy link
Contributor

mbgl::GeometryCollection can potentially contain significant amount of data, however before this change we had several places where GeometryCollection instances were unnecessarily copied causing extra allocations, which affected the performance. In particular, queryRenderedFeatures API performance was affected (see #15183).

@pozdnyakov pozdnyakov force-pushed the mikhail_refactor_geometry_collection branch from a890d09 to cadc2d2 Compare July 24, 2019 12:07
@pozdnyakov pozdnyakov self-assigned this Jul 24, 2019
@pozdnyakov pozdnyakov added Core The cross-platform C++ core, aka mbgl performance Speed, stability, CPU usage, memory usage, or power usage labels Jul 24, 2019
@pozdnyakov pozdnyakov force-pushed the mikhail_refactor_geometry_collection branch from cadc2d2 to 722967f Compare July 24, 2019 12:46
Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

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

LGTM

@julianrex
Copy link
Contributor

For reference, testing with a modified version of #15203.

Before:
Image 2019-07-24 at 11 16 06 AM

After:
image

@pozdnyakov pozdnyakov merged commit 2c2e858 into master Jul 24, 2019
@pozdnyakov pozdnyakov deleted the mikhail_refactor_geometry_collection branch July 24, 2019 18:05
@@ -57,32 +57,31 @@ std::vector<GeometryCollection> classifyRings(const GeometryCollection& rings) {
std::size_t len = rings.size();

if (len <= 1) {
polygons.push_back(rings);
polygons.emplace_back(rings.clone());
return polygons;
Copy link
Contributor

Choose a reason for hiding this comment

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

I briefly checked in debugger - in majority of cases len == 1.
Following how classifyRings is used for (auto& polygon : classifyRings(geometry)) { suggest to redesign clasifyRings so that it doesn't clone() but access the same const reference.

@friedbunny friedbunny added this to the release-queso milestone Jul 24, 2019
@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl needs changelog Indicates PR needs a changelog entry prior to merging. performance Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants