[core, ios] Improve performance of queryRenderedFeatures by reducing # of temp GeometryCollection #15183

julianrex · 2019-07-19T18:41:34Z

This PR aims to improve performance of queryRenderedFeatures by reducing the number of temporary GeometryCollection that are allocated.

getGeometries() is now called once, rather than 3 times; it is passed in to subsequent functions
offsetLine(...) now returns a std::unique_ptr - beforehand the copy-constructor & destructor were being called...a lot. (This is the main improvement).
iosapp has been updated with a menu entry so that we can test these improvements (and future improvements).

Note - this does NOT have @pozdnyakov's suggestion of using mbgl::Immutable - I started, but I'd need to pair on this - perhaps we can do this as a follow-on PR?

julianrex · 2019-07-19T21:30:16Z

@tobrun can you take a look at this and see what changes are needed for Android.

pozdnyakov · 2019-07-21T22:05:49Z

src/mbgl/renderer/layers/render_line_layer.cpp

+
+    if (offset == 0) return nullptr;
+
+    std::unique_ptr<GeometryCollection> newRings = std::make_unique<GeometryCollection>();


After some checks I came to conclusion that we could keep the old semantics with the optional return type.
The problem was caused by implicit copy of the local variable on return, that can be fixed just with adding std::move() call (RVO is failing here as local and return types are different).

optional<GeometryCollection> offsetLine() { GeometryCollection newRings; return newRings; // Copy here! } optional<GeometryCollection> offsetLine() { GeometryCollection newRings; return std::move(newRings); // No copy :) }

Gotcha - will test out.

return std::move(newRings); will prevent copy elision, I think we even have clang check for that.

Another option is to return optional<GeometryCollection>(std::move(newRings));

hm, another issue could be that we have two return statements that could prevent RVO.

return std::move(newRings); will prevent copy elision

in this case we're creating an instance of another type: optional<GeometryCollection>. Returning rvalue triggers converting move constructor; returning lvalue triggers converting copy constructor.

julianrex · 2019-07-22T02:30:35Z

@pozdnyakov can you help with the node changes? I started to try to understand what needs to be changed but very quickly hit a road block. Would the change back to optional potentially fix the node tests that are failing?

tobrun · 2019-07-22T07:57:05Z

@tobrun can you take a look at this and see what changes are needed for Android.

afaik none at the moment

pozdnyakov · 2019-07-22T09:22:11Z

@pozdnyakov can you help with the node changes? I started to try to understand what needs to be changed but very quickly hit a road block. Would the change back to optional potentially fix the node tests that are failing?

The proposed change had slightly affected logic in RenderLineLayer::queryIntersectsFeature, which caused render tests failures. Fixed now in 4f5dac6 (it also contains some further refactoring to offsetLine function).

julianrex · 2019-07-22T14:04:25Z

Just forced pushed, which overwrites @pozdnyakov's 4f5dac6 - running through Instruments shows that using optional is slower than std::unique_ptr. Timing went down from an average of 179ms to 128ms (on an iPhone X).

Unfortunately the low average we were seeing earlier was due to the bug mentioned above - not making enough calls :(

julianrex · 2019-07-22T14:46:46Z

New low average is around 75ms (Both Release and RelWithDebInfo) - perhaps due to switch to emplace_back (or perhaps the 128ms was a trace using a old framework build).

…ough

julianrex · 2019-07-22T15:04:22Z

Rebased.

pozdnyakov · 2019-07-22T15:29:09Z

Optional: could we squash some commits?

alexshalamov · 2019-07-22T17:24:13Z

src/mbgl/tile/geometry_tile_data.cpp

@@ -112,8 +112,6 @@ static Feature::geometry_type convertGeometry(const GeometryTileFeature& geometr
        );
    };

-    GeometryCollection geometries = geometryTileFeature.getGeometries();


@julianrex @pozdnyakov was there a need to pass const GeometryCollection& geometries all the way here when we already have constref to geometryTileFeature?

I think the problem was on line 115 where we made copy. Should have been:

const GeometryCollection& geometries = geometryTileFeature.getGeometries();

alexshalamov · 2019-07-22T17:42:50Z

src/mbgl/renderer/layers/render_line_layer.cpp

+
+    if (offset == 0) return nullptr;
+
+    std::unique_ptr<GeometryCollection> newRings = std::make_unique<GeometryCollection>();


return std::move(newRings); will prevent copy elision, I think we even have clang check for that.

Another option is to return optional<GeometryCollection>(std::move(newRings));

alexshalamov · 2019-07-22T17:43:26Z

platform/ios/CHANGELOG.md

@@ -8,6 +8,9 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT

 * Fixed flickering on style change for the same tile set ([#15127](https://github.com/mapbox/mapbox-gl-native/pull/15127))

+### Other changes
+
+* Improved feature querying performance (focussed on roads). ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))


focused? 😄 I've never used focussed.

UK English ;)

If even the Europeans are against you... 😅

In any case, this also struck me as wrong. I think we should stick with ‘merican English, given our audience/company/etc.

Alternatively:

Suggested change

* Improved feature querying performance (focussed on roads). ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))

* Improved feature querying performance of road features. ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))

alexshalamov · 2019-07-22T18:09:42Z

src/mbgl/renderer/layers/render_line_layer.cpp

@@ -265,13 +268,14 @@ bool RenderLineLayer::queryIntersectsFeature(
                          .evaluate(feature, zoom, style::LineOffset::defaultValue()) * pixelsToTileUnits;

    // Apply offset to geometry
-    auto offsetGeometry = offsetLine(feature.getGeometries(), offset);
+    auto offsetGeometry = offsetLine(geometries, offset);


const auto& offsetGeometry = offsetLine(geometries, offset);

julianrex · 2019-07-22T19:51:05Z

Chatting with @alexshalamov - we're going to continue investigating this.

friedbunny · 2019-07-22T20:02:59Z

platform/ios/CHANGELOG.md

@@ -8,6 +8,9 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT

 * Fixed flickering on style change for the same tile set ([#15127](https://github.com/mapbox/mapbox-gl-native/pull/15127))

+### Other changes
+
+* Improved feature querying performance (focussed on roads). ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))


If even the Europeans are against you... 😅

In any case, this also struck me as wrong. I think we should stick with ‘merican English, given our audience/company/etc.

Alternatively:

Suggested change

* Improved feature querying performance (focussed on roads). ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))

* Improved feature querying performance of road features. ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))

friedbunny · 2019-07-22T20:05:13Z

platform/ios/app/MBXViewController.m

@@ -17,6 +17,8 @@
 #import "../src/MGLMapView_Experimental.h"

 #import <objc/runtime.h>
+#import <os/log.h>
+#import <os/signpost.h>


iosapp is larded down and we’ve been trying to trim it back — since this logging functionality could presumably be useful in other/future view controllers, can we move this stuff out into a separate file?

alexshalamov · 2019-07-23T08:48:36Z

@julianrex @pozdnyakov pushed some experiments to https://github.com/mapbox/mapbox-gl-native/compare/alexshalamov_wip_qrf

Not sure why 'unique_ptr' version is faster than construction / elision of geometrycollection on stack.

julianrex · 2019-07-23T15:05:19Z

Summarizing chat with @alexshalamov & @pozdnyakov: we are going to split this PR into 2 new ones:

one for the iOS "test": [ios] Add road query test and signpost macros #15203
one for the GeometryCollection improvements: [core] Avoid geometry collections copying #15201

@friedbunny I will address your comments in the new PR!

julianrex added iOS Mapbox Maps SDK for iOS Core The cross-platform C++ core, aka mbgl querying Issues with querySourceFeatures or queryRenderedFeatures labels Jul 19, 2019

julianrex added this to the release-q milestone Jul 19, 2019

julianrex requested review from pozdnyakov, alexshalamov and a team July 19, 2019 18:41

julianrex added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jul 19, 2019

julianrex force-pushed the jrex/qrf-road-features branch from 35bd951 to bd77689 Compare July 19, 2019 21:04

julianrex force-pushed the jrex/qrf-road-features branch from bd77689 to 5568810 Compare July 20, 2019 04:45

pozdnyakov reviewed Jul 21, 2019

View reviewed changes

julianrex force-pushed the jrex/qrf-road-features branch from 4f5dac6 to a534dd3 Compare July 22, 2019 14:01

julianrex removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jul 22, 2019

julianrex requested a review from 1ec5 as a code owner July 22, 2019 15:01

julianrex removed the request for review from 1ec5 July 22, 2019 15:01

Julian Rex added 9 commits July 22, 2019 11:02

[ios] Profiling test for querying road features

4f2cc3a

[ios] Changed profile to use RelWithDebInfo configuration

90b4314

[ios] Perf experiments

ed3ef82

Avoid memory leak

1844b9c

Address commit feedback & remove $$JR tag.

43c695b

[ios] Fix leak.

b7ee698

[ios] Clean up iosapp / Add separate entry for road query.

1380f12

[core] Change to nullptr

c21f06f

[core] Remove commented code.

47310ea

Julian Rex added 4 commits July 22, 2019 11:02

Fixed bug with polygonIntersectsBufferedMultiline not being called en…

f923e91

…ough

Call queryRoads 10 times in a row.

9624904

[core] Switched to emplace_back

8ebc0d6

Change log entries

a3f7bfd

julianrex force-pushed the jrex/qrf-road-features branch from 31bec80 to a3f7bfd Compare July 22, 2019 15:04

pozdnyakov approved these changes Jul 22, 2019

View reviewed changes

alexshalamov reviewed Jul 22, 2019

View reviewed changes

julianrex added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jul 22, 2019

friedbunny reviewed Jul 22, 2019

View reviewed changes

julianrex closed this Jul 23, 2019

pozdnyakov mentioned this pull request Jul 24, 2019

[core] Avoid geometry collections copying #15201

Merged

julianrex mentioned this pull request Jul 24, 2019

[ios] Add road query test and signpost macros #15203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core, ios] Improve performance of queryRenderedFeatures by reducing # of temp GeometryCollection #15183

[core, ios] Improve performance of queryRenderedFeatures by reducing # of temp GeometryCollection #15183

julianrex commented Jul 19, 2019

julianrex commented Jul 19, 2019

pozdnyakov Jul 21, 2019

julianrex Jul 22, 2019

alexshalamov Jul 22, 2019

alexshalamov Jul 22, 2019

pozdnyakov Jul 23, 2019

julianrex commented Jul 22, 2019

tobrun commented Jul 22, 2019

pozdnyakov commented Jul 22, 2019

julianrex commented Jul 22, 2019

julianrex commented Jul 22, 2019

julianrex commented Jul 22, 2019

pozdnyakov commented Jul 22, 2019

alexshalamov Jul 22, 2019

alexshalamov Jul 22, 2019

alexshalamov Jul 22, 2019 •

edited

Loading

julianrex Jul 22, 2019

friedbunny Jul 22, 2019 •

edited

Loading

alexshalamov Jul 22, 2019

julianrex commented Jul 22, 2019

friedbunny Jul 22, 2019 •

edited

Loading

friedbunny Jul 22, 2019

alexshalamov commented Jul 23, 2019

julianrex commented Jul 23, 2019 •

edited

Loading


		if (offset == 0) return nullptr;

		std::unique_ptr<GeometryCollection> newRings = std::make_unique<GeometryCollection>();

	* Improved feature querying performance (focussed on roads). ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))
	* Improved feature querying performance of road features. ([#15183](https://github.com/mapbox/mapbox-gl-native/pull/15183))

[core, ios] Improve performance of queryRenderedFeatures by reducing # of temp GeometryCollection #15183

[core, ios] Improve performance of queryRenderedFeatures by reducing # of temp GeometryCollection #15183

Conversation

julianrex commented Jul 19, 2019

julianrex commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex commented Jul 22, 2019

tobrun commented Jul 22, 2019

pozdnyakov commented Jul 22, 2019

julianrex commented Jul 22, 2019

julianrex commented Jul 22, 2019

julianrex commented Jul 22, 2019

pozdnyakov commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshalamov Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex commented Jul 22, 2019

friedbunny Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshalamov commented Jul 23, 2019

julianrex commented Jul 23, 2019 • edited Loading

alexshalamov Jul 22, 2019 •

edited

Loading

friedbunny Jul 22, 2019 •

edited

Loading

friedbunny Jul 22, 2019 •

edited

Loading

julianrex commented Jul 23, 2019 •

edited

Loading