[not ready] anno remove bug fix & in-app settings to flex annotations

[incanus]

Add a GeoJSON with ~10,000 point features and allow for addition of 100, 1000, or 10000 of them in-app, plus removal.

Fixes a crasher in annotation removal.

Needs some work for removal of 10000 features per:

[incanus]

Moving to std::unordered_map for AnnotationManager::annotationTiles and Annotation::tileFeatures doesn't appear to help with the perf problem on removing 10,000 annotations.

Hash used for Tile::ID map keys:

struct Hash {
    std::size_t operator()(ID const& i) const {
        return std::hash<uint64_t>()(i.to_uint64());
    }
};

[mb12]

You can use an intrusive list and return the actual object pointer as annotation ID. Deletion is O (1) for intrusive lists.

[incanus]

Not sure I follow @mb12 — the bottleneck is on iteration of the annotationTiles map.

[mb12]

My mistake. I was referring to annotations map in AnnotationManager. It could be safely converted to an intrusive list if it were a performance bottleneck.

[mb12]

For point annotations you can skip the inner loop once it's annotation tile is found. A point annotation can lie on only one tile. This should help a bit.

Full iteration through all tiles is needed only for lines and areas which get clipped into multiple tiles.

[mb12]

We can also delete the annotation tile itself in the inner loop if all its annotations have been deleted.

[incanus]

Well, point annotations are in one tile per zoom level. But I could use that knowledge to maybe make things smarter.

[mb12]

We can also use the following approach for handling point annotations.

1.) With every point annotation, save also the actual latitude, longitude during the add call (if not doing so already).
2.) At time of deletion, convert the lat long to a list of Tile::IDs (one tile id for each zoom).
3.) Instead of iterating through all annotationTiles, look for only these tile IDs we just computed.

[incanus]

Great idea. The getPoint() call will work perfectly for this.

[incanus]

Ok, I was able to restructure things a bit in the way we store live tile features so that removal is much faster at annotation delete time.

First improvement in 48d70c4 avoids a high-overhead iteration of all of the annotation manager's tiles for each annotation by calculating an annotation's intersected tile in each zoom upfront, since this is predictable. It uses the same projection math done at annotation creation time, but the overhead is low. Based on @mb12's recommendation in #1061 (comment).

Second improvement in b4daad5 avoids a simple assignment operator penalty, which was the new bottleneck.

That still left an unacceptable time (~10s) for removal of 10,000 annotations on an iPhone 5s. The third improvement in ee2e32f moves from a vector of live tile features in a layer to an unordered map keying on a unique std::size_t. This number is stored in the annotation object instead of a weak pointer to the feature itself, meaning deletion lookups later are trivial because they are by key instead of by iteration of all tile features until the desired one.

Other thing I'm a little fuzzy on is the adaption of the interface method getFeature() which is called with a vector index during parsing. To return the appropriate value from the new map I advance an iterator the appropriate distance:

util::ptr<const GeometryTileFeature> LiveTileLayer::getFeature(std::size_t i) const {
    auto it = features.begin();
    std::advance(it, i);
    return it->second;
}

Review is welcome, particularly by @jfirebaugh since he made getFeature() originally.

I saw, and still see, near-instant adds of 100, 1000, or 10000 annotations, and the noticeable delay for removal of the 10,000 went from over 13 seconds in the original ticket to less than 2 seconds. Considering the 10,000 annotations is meant to be a stress test and not a common use case, I think we're good here perf-wise with a speedup of 6.5x.

[incanus]

BTW the biggest delay here for adds is actually the GeoJSON parsing of 10,000 features, not the addition & rendering in GL.

[incanus]

Eh, I'm not super happy with the rendering tradeoffs here. The 10,000 annotations are now visibly tiling when before they weren't. I suspect it's the move to std::unordered_map and the getFeature() atop it since unordered maps are generally slower at iterated subranges, but I'll be profiling soon to figure out for sure.

I would want to wait on landing these perf improvements until we get @kkaefer's general API improvements from #1022 landed, even if we don't yet fix the threading problem there. See my latest comment in #1022 (comment).

[incanus]

I suspect it's the move to std::unordered_map and the getFeature() atop it since unordered maps are generally slower at iterated subranges

Ayup.

[incanus]

A map isn't any better.

[incanus]

Ahhh, but map or unordered_map, this is a much faster way to access a feature by index:

util::ptr<const GeometryTileFeature> LiveTileLayer::getFeature(std::size_t i) const {
    return features.find(i)->second;
}

[incanus]

I don't think moving from a vector to a map of any variety for a LiveTileLayer's features is a good move since we have to iterate through them by index. I looked into not doing this, but it doesn't make sense to have the interface e.g. grab all features upfront when they are serially packed a pbf over in our parent VectorTileLayer. And there's no getting around the complications of iterating a map by index when also trying to remain const since we can't maintain state with a class member iterator.

Currently trying going back to a vector, then looking into building a map of features to delete (and what tiles) when iterating annotations, then deleting them en masse once done iterating on a per-tile basis.

[incanus]

Was the switch from std::vector to std::map to improve deletion performance of specific annotationId in tileAnnotations container?

No, to improve deletion performance of specific features from a LiveTileLayer. Doing this by iterating every feature every time you need to delete one was taking too long. If you store a map mapping the annotation to a feature key instead, this is much faster to delete by key later.

[mb12]

Can you convert LiveTileLayer::features to an intrusive list? Basically the next and prev pointer will be come part of LiveTileFeature making deletion and insertion an O(1) operation.

[incanus]

If I'm understanding right, this runs into problems similar to those described in #1061 (comment) ("complications") because feature getting should be const.

[mb12]

Iteration by index is not really needed. Its just a sequential iteration of all objects in the container both in TileParser and SymbolBucket.

[incanus]

Iteration by index is not really needed. Its just a sequential iteration of all objects in the container both in TileParser and SymbolBucket.

Right. But what is a good way to do this?

[jfirebaugh]

For AnnotationManager::tiles, replace std::vector<uint32_t> with std::unordered_set<uint32_t>, so that removal can be done with erase rather than std::erase_if.

Why is the mapped_type of Annotation::tileFeatures a vector? Isn't there always exactly one LiveTileFeature per Annotation per Tile::ID? If that's the case, then tileFeatures can be std::unordered_map<Tile::ID, std::size_t>, where the std::size_t is an index into the LiveTileLayer::features vector.

Beyond these two things, the more fundamental issue is that the APIs of AnnotationManager::removeAnnotations and LiveTileLayer::getFeature can't simultaneously be efficient. The removeAnnotations supports removal of an arbitrary set of annotation IDs, while getFeature assumes an vector-based implementation for features. So removeAnnotations of n annotations requires n deletions from the features vector, which is quadratic. We need to either change the annotation API or change the GeometryTileLayer API. My preference is the former: I think the annotations API is too flexible for its own good. A more layer-oriented API, where you add a batch of annotations as a layer, and can remove the entire layer (or reset its data wholesale), but not change individual elements, would be simpler in implementation and more convenient for developers in most cases.

[mb12]

Instead of getFeature, provide an iterator that iterates through all Features in the TileLayer.

[incanus]

Instead of getFeature, provide an iterator that iterates through all Features in the TileLayer.

@mb12 Right, tried this per above. The problem is getFeature() needs to be const because a reference to a const layer is passed to the parsing step that gets those features.

Why is the mapped_type of Annotation::tileFeatures a vector? Isn't there always exactly one LiveTileFeature per Annotation per Tile::ID?

@jfirebaugh Sure, for points. We'll be doing shapes, too.

A more layer-oriented API, where you add a batch of annotations as a layer, and can remove the entire layer (or reset its data wholesale), but not change individual elements, would be simpler in implementation and more convenient for developers in most cases.

I've kept everything layered this way because shapes will eventually need their own layer each for per-annotation styling (so, filter expressions). Points can all go into a single layer, but I suppose they don't have to. Was just trying to keep things simple organizationally.

I think splitting point annotations into multiple layers will get complicated when it comes to selection/picking, as well as with rendering when z-ordering issues come into play. I'm not too worried about gestures on shape annotations; even Apple doesn't have this.

[incanus]

For AnnotationManager::tiles, replace std::vector<uint32_t> with std::unordered_set<uint32_t>, so that removal can be done with erase rather than std::erase_if.

Nice, I like it. This is my inexperience with unordered_set talking.

[incanus]

Use of unordered_set gives me an idea for using this internally for LiveTileLayer features since order doesn't matter.

[mb12]

If the returns value of getFeatures is a const_iterator wouldn't that ensure that doing iter or iter() return a const T.

[incanus]

Use of unordered_set gives me an idea for using this internally for LiveTileLayer features since order doesn't matter.

Eh, hashing a util::ptr<const LiveTileFeature> is kind of a pain.

If the returns value of getFeatures is a const_iterator wouldn't that ensure that doing iter or iter() return a const T.

@mb12 Lemme check it out.

[incanus]

We're currently at about 3 seconds to remove 10,000 annotations from an iPhone 5s. I think we've made enough improvements here, actually. Lots of little stuff, and although the paradox that @jfirebaugh outlines may still hold, it's not worth focusing on so much right now, especially given that the annotations model may change when we move to shapes. I'm cool to squash the improvements here and merge soon.

[jfirebaugh]

@mb12 getFeatures can't return a const_iterator because that would tie the abstract GeometryTileLayer interface to a specific implementation and concrete GeometryTileFeature derived class.

[jfirebaugh]

Why is the mapped_type of Annotation::tileFeatures a vector? Isn't there always exactly one LiveTileFeature per Annotation per Tile::ID?

@jfirebaugh Sure, for points. We'll be doing shapes, too.

Seems like it should be true for shapes too.

[incanus]

Seems like it should be true for shapes too.

Right, I was forgetting that LiveTileFeature's geometries is a collection, so this covers holes or whatnot. Will remedy.

[mb12]

@jfirebaugh Yes, we would have to create an iterator that wraps around the underlying container (vector, map, intrusive list, unordered_map) iterator and return it instead of vector.begin().

[incanus]

Looking like this:

https://dl.dropboxusercontent.com/u/575564/100%2C%201000%2C%2010000.mp4

[incanus]

Going to wait for tests to pass here, then merge this and leave perf alone for a while.

[incanus]

Oh, gotta merge master.

[incanus]

Squashed over in #1076.