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

[iOS][WIP] Improve annotationsView performances #6840

Closed
wants to merge 1 commit into from
Closed

[iOS][WIP] Improve annotationsView performances #6840

wants to merge 1 commit into from

Conversation

naluhh
Copy link

@naluhh naluhh commented Oct 27, 2016

This PR adresses the following issue:

What changed ?

updateAnnotationViews was not working properly. The delegate method was called on each frame even if it wasn't in the view work. As a result, performance were worse when an annotation was outside the screen (!).
The reuse queue was also an NSArray instead of a NSSet resulting in poor performance when picking inside.

This PR improve the performance of the updateAnnotationViews especially when you are using complicated views.

One possible improvement to this would be to use queryPointAnnotations, but it's too slow when there is a lot of annotations on the screen.

Testing done

I tested with thousand views and CAAnimation spread across the world with much better performances.

cc @1ec5

@mention-bot
Copy link

@naluhh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @boundsj, @1ec5 and @friedbunny to be potential reviewers.

@boundsj
Copy link
Contributor

boundsj commented Oct 27, 2016

Thanks for the contribution @naluhh! I will review as part of #5987 that I'm currently working on again now that #6055 is fixed.

@boundsj boundsj mentioned this pull request Oct 31, 2016
2 tasks
@boundsj
Copy link
Contributor

boundsj commented Nov 1, 2016

Thanks again for your PR @naluhh. I went with the implementation in #5987 because we wanted a solution that relies on the more efficient query mechanism in the mbgl (C++) core. Also, I wanted to stay away from implementations that rely on changing the visibility of the UIView objects. I was also concerned about using a set for the queue since we would not want to store only distinct views based on a hash -- we want to be able to reuse any view that has been created previously.

I understand this PR also introduced a new public API on MGLAnnotationView called willBeEnqueued. I'm not sure I see the value of that compared to the existing prepareForReuse API that is also more familiar to users that have experience with similar APIs (like MapKit's and UITableView). It may also leak more than necessary about the underlying reuse mechanism. Please do feel free to submit another PR with the changes for willBeEnqueued if you feel you can make the case for it.

Thanks again!

@boundsj boundsj closed this Nov 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants