Optimize annotation view updates #5987

boundsj · 2016-08-13T00:24:50Z

Now that #5165 has landed, we can address the issues reported in #5787 and again in #5489 (comment). This PR will:

Only update annotations in that are backed by MGLAnnotationView subclasses
Make use of queryPointAnnotations (from Replace getPointAnnotationsInBounds() w/ queryPointAnnotations() #5165) to operate on potentially less view backed annotations when updating them

boundsj · 2016-08-13T00:27:17Z

This first commit addresses #5787 specifically. After this change, profiling shows that drawing icons at the GL level is responsible for most of the
CPU usage when 10k annotations backed by sprites are added to the map and then the map is continuously panned.

cc @incanus

boundsj · 2016-08-29T20:02:32Z

Noting that the parts of this PR not blocked by #6055 have been PRd in #6199

- Get sets of visible and offscreen annotations and update and enqueue as required - Query viewport adjusted if tilted (avoid apparent issue with queryPointAnnotations when the query box is larger than the actual viewport) - Add a small debugging display in iOS app to see annotations going in and out of the reuse queue

This works around a performance issue that made getting an annotation context expensive. It also works around an issue with the underlying mbgl query so that even if it (rarely) returns an incorrect result, the correct visual effect still occurs and the reuse queue is added to and drained as expected.

But implement it with _annotationContextsByAnnotation to avoid the expensive lookup in annotationTagForAnnotation:

boundsj · 2016-10-31T06:46:15Z

The fix of #6055 freed us up to make some long desired changes to the way annotation views are updated and queued for reuse. The summary of changes here so far is:

Use query point annotations to create subsets of visible and offscreen annotations
Update the centers of the visible annotations as the map is moved
Dequeue annotations that are offscreen
Work around and a few (what seems to be) remaining issues / limitations with symbol querying in mbgl core where the no annotations come back in the query -- sometimes this seems to happen randomly but rarely during rotation / changing zoom. It also seems to be a limitation that a query box larger than the viewport becomes increasingly problematic as pitch increases. But again, this PR works around those issues so that the performance benefit of using mbgl's query can still be used
A performance refactor to avoid MGLMapView annotationTagForAnnotation: since the linear performance characteristics of that method become problematic with thousands of annotation views. The tradeoff is space complexity and another std::map to make annotation context retrieval time constant

When testing for 1 minute with 10,000 annotation views, high zoom (to trigger heavy reuse queue usage), and a constant panning gesture, the performance increase is significant:

Before: total weight of ~50 seconds and `updateAnnotationViews` being ~88% of that

Now: total weight of ~15 seconds and `updateAnnotationViews` being ~50% of that

This commit also includes an optional debugger view in iosapp to confirm the reuse queue is working as expected:

This addresses concerns raised in #5787 and #6173. I believe it is a more desirable and complete solution than the one proposed in #6840.

cc @1ec5 @incanus @frederoni

boundsj · 2016-10-31T06:48:44Z

Noting also that while this is not a complete fix for #5489, I'm seeing much better results when scrolling since, I think, the main thread as a whole is taxed far less than before.

1ec5 · 2016-10-31T16:41:28Z

platform/ios/src/MGLMapView.mm

-typedef std::map<MGLAnnotationTag, MGLAnnotationContext> MGLAnnotationContextMap;
+typedef std::map<MGLAnnotationTag, MGLAnnotationContext> MGLAnnotationTagContextMap;
+
+typedef std::map<id<MGLAnnotation>, MGLAnnotationContext> MGLAnnotationObjectContextMap;


What if we instead have a mapping from annotation objects to annotation tags? It would be an additional hop, but also less parallel state to keep in sync.

Good idea. Done in fc8d77a

1ec5 · 2016-10-31T16:42:40Z

platform/ios/src/MGLMapView.mm

@@ -129,7 +129,9 @@ typedef NS_ENUM(NSUInteger, MGLUserTrackingState) {

 /// Mapping from an annotation tag to metadata about that annotation, including
 /// the annotation itself.
-typedef std::map<MGLAnnotationTag, MGLAnnotationContext> MGLAnnotationContextMap;
+typedef std::map<MGLAnnotationTag, MGLAnnotationContext> MGLAnnotationTagContextMap;


Rename this type in the macOS implementation as well to keep the two implementations from diverging too much.

Done in fc8d77a

This creates an extra hop to get the context but eliminates the need to sync two sets of references to each context object. This also syncs up the ios and macos names (and types) for MGLAnnotationTagContextMap

1ec5 · 2016-10-31T23:40:22Z

platform/ios/src/MGLMapView.mm

@@ -189,6 +192,7 @@ - (void)accessibilityDecrement
    MGLAnnotationAccessibilityElement *accessibilityElement;
    MGLAnnotationView *annotationView;
    NSString *viewReuseIdentifier;
+    MGLAnnotationTag annotationTag;


This is unused, right?

Fixed in 8e67bef

1ec5 · 2016-11-02T07:44:45Z

platform/ios/app/MBXViewController.m

@@ -1506,9 +1527,24 @@ - (void)mapView:(MGLMapView *)mapView tapOnCalloutForAnnotation:(id <MGLAnnotati
 - (void)mapView:(MGLMapView *)mapView didFinishLoadingStyle:(MGLStyle *)style
 {
    // Default Mapbox styles use {name_en} as their label language, which means
+    NSUInteger queuedAnnotations = 0;


Looks like a bad merge occurred.

Thanks. Fixed in #6882

Streamlined -[MGLMapView annotationTagForAnnotation:] to use the mapping added in #5987. Reverted an unsafe access into that mapping that was added in #5987; returning nil is preferable when no such annotation can be found. Return the user location annotation view when given the user location annotation.

boundsj added iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Aug 13, 2016

boundsj added this to the ios-v3.4.0 milestone Aug 13, 2016

boundsj self-assigned this Aug 13, 2016

boundsj force-pushed the boundsj-optimize-annotation-updates branch from f44cdeb to bcc3d9c Compare August 14, 2016 03:05

boundsj added annotations Annotations on iOS and macOS or markers on Android refactor labels Aug 15, 2016

This was referenced Aug 17, 2016

Map::queryPointAnnotations() -> Map::queryRenderedFeatures() returns unexpected result as map is moved #6055

Closed

Map view lags behind annotation views when panning #5489

Closed

boundsj mentioned this pull request Oct 27, 2016

[iOS][WIP] Improve annotationsView performances #6840

Closed

boundsj added bug release blocker Blocks the next final release labels Oct 27, 2016

boundsj changed the title ~~[WIP] Optimize annotation view updates~~ Optimize annotation view updates Oct 28, 2016

boundsj added 6 commits October 30, 2016 22:42

[ios] Restore viewForAnnotation:

1f0efff

But implement it with _annotationContextsByAnnotation to avoid the expensive lookup in annotationTagForAnnotation:

[ios] Update "by tag" map's context when the view changes

5c09b17

[ios] Add switch for reuse queue debugging stats HUD

25e253f

[ios] Fix bad merge in iosapp

a8c481b

boundsj force-pushed the boundsj-optimize-annotation-updates branch from d374b45 to a8c481b Compare October 31, 2016 05:59

boundsj changed the base branch from master to release-ios-v3.4.0 October 31, 2016 06:16

boundsj added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Oct 31, 2016

1ec5 suggested changes Oct 31, 2016

View reviewed changes

boundsj mentioned this pull request Oct 31, 2016

Major bug with annotation views #6864

Closed

[ios, macos] Use annotation tags by annotation object map

fc8d77a

This creates an extra hop to get the context but eliminates the need to sync two sets of references to each context object. This also syncs up the ios and macos names (and types) for MGLAnnotationTagContextMap

1ec5 suggested changes Oct 31, 2016

View reviewed changes

[ios] Remove annotationTag property on context

8e67bef

1ec5 approved these changes Nov 1, 2016

View reviewed changes

boundsj mentioned this pull request Nov 1, 2016

Accessor for visible annotation views in the viewport #5467

Closed

[ios] Updated changelog

946f014

boundsj merged commit 2901bc0 into release-ios-v3.4.0 Nov 1, 2016

boundsj mentioned this pull request Nov 1, 2016

MGLMapView calls MGLMapViewDelegate 'viewForAnnotation:' for annotations outside visible area #6173

Closed

boundsj deleted the boundsj-optimize-annotation-updates branch November 1, 2016 19:01

boundsj removed the ✓ ready for review label Nov 1, 2016

1ec5 reviewed Nov 2, 2016

View reviewed changes

1ec5 mentioned this pull request Nov 8, 2016

-[MGLMapView viewForAnnotation:] crashes when given annotation not on the map — map::at: key not found #6953

Closed

1ec5 mentioned this pull request Nov 8, 2016

Fix crash getting annotation view #6957

Merged

yojique added a commit to yojique/mapbox-gl-native that referenced this pull request Dec 1, 2016

[ios] fix changes from mapbox#5987

048db99

boundsj mentioned this pull request Apr 3, 2017

iOS Annotation view sticked at the left of the screen when dragging map #8603

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize annotation view updates #5987

Optimize annotation view updates #5987

boundsj commented Aug 13, 2016 •

edited

Loading

boundsj commented Aug 13, 2016

boundsj commented Aug 29, 2016

boundsj commented Oct 31, 2016 •

edited

Loading

boundsj commented Oct 31, 2016

1ec5 Oct 31, 2016

boundsj Oct 31, 2016

1ec5 Oct 31, 2016

boundsj Oct 31, 2016

1ec5 Oct 31, 2016

boundsj Oct 31, 2016

1ec5 Nov 2, 2016

boundsj Nov 2, 2016

Optimize annotation view updates #5987

Optimize annotation view updates #5987

Conversation

boundsj commented Aug 13, 2016 • edited Loading

boundsj commented Aug 13, 2016

boundsj commented Aug 29, 2016

boundsj commented Oct 31, 2016 • edited Loading

Before: total weight of ~50 seconds and updateAnnotationViews being ~88% of that

Now: total weight of ~15 seconds and updateAnnotationViews being ~50% of that

boundsj commented Oct 31, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boundsj commented Aug 13, 2016 •

edited

Loading

boundsj commented Oct 31, 2016 •

edited

Loading

Before: total weight of ~50 seconds and `updateAnnotationViews` being ~88% of that

Now: total weight of ~15 seconds and `updateAnnotationViews` being ~50% of that