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

[core] annotation manager thread safety #9208

Closed
wants to merge 3 commits into from

Conversation

ivovandongen
Copy link
Contributor

@ivovandongen ivovandongen commented Jun 7, 2017

Cuts up the annotation manager so is thread safe (#8820);

  • moves data into the source
  • moves the tile generation into the render source
  • style updates are done immediately if a current style is loaded and/or done when a (new) style is loaded instead of mutating the style on render

TODO:

  • Moving the data into the Source means it is copied on every mutation, this is less performant for bulk operations. We should probably support bulk operations explicitly to avoid multiple sequential copies in those cases.
  • Currently some platforms (ios/macos most notably) execute fairly heavy UI operations on SourceObserver::onSourceChanged callbacks. When mutating the AnnotationSource rapidly, this is quite the bottleneck. Since these callbacks are needed to signal changes for style diffing, we need to make sure any other operations either use a different signal or guard better from redundantly executing.

Fixes:

@ivovandongen ivovandongen self-assigned this Jun 7, 2017
@kkaefer kkaefer added Core The cross-platform C++ core, aka mbgl refactor labels Jun 7, 2017
AnnotationManager::~AnnotationManager() = default;

AnnotationID AnnotationManager::addAnnotation(const Annotation& annotation, const uint8_t maxZoom) {
AnnotationID AnnotationManager::addAnnotation(const Annotation &annotation, const uint8_t maxZoom) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert the & whitespace changes?

@jfirebaugh
Copy link
Contributor

Moving the data into the Source means it is copied on every mutation, this is less performant for bulk operations. We should probably support bulk operations explicitly to avoid multiple sequential copies in those cases.

I'm not sure that this is going to be a viable approach. The SDKs already expose non-bulk APIs, right? If those APIs change their performance in a major way, such that existing workloads become much slower, and the only way to recover performance is to switch to bulk APIs, then I would consider that a breaking change.

I think in order for the immutable approach to be viable, copies must be fairly cheap. So, either:

  • what you're copying needs to be smallish (e.g. Layers only copy their properties -- not too bad)
  • you use immutable datastructures all the way down, so only a small portion of the overall structure needs to be copied, and the rest remains shared.

The second is possible in this case, but would need a sizable investment in immutable datastructures -- we'd need an immutable rtree implementation, for one.

This approach also feels to me like it takes us further away from where we want AnnotationManager to end up in the longer term, rewritten in terms of public APIs. If we were to rewrite it in terms of runtime styling and custom sources, then that will be pull-oriented model rather than push-oriented: mbgl core will ask the custom source for tile data as needed, rather than having the custom source update existing tiles in response to annotations being added.

Since AnnotationManager doesn't return any references to internal data -- in particular getTileData always creates a fresh std::unique_ptr<AnnotationTileData> -- an alternative is to add internal locking.

@ivovandongen
Copy link
Contributor Author

@jfirebaugh

an alternative is to add internal locking.

Ok. Continuing in #9220

@ivovandongen ivovandongen deleted the annotations-thread-safety branch June 8, 2017 09:57
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 refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants