From 986465e015132dccb5812583b4b1d05083ed9db6 Mon Sep 17 00:00:00 2001
From: Mikhail Pozdnyakov <mikhail.pozdnyakov@mapbox.com>
Date: Mon, 18 Nov 2019 21:24:09 +0200
Subject: [PATCH] [core] Introduce style::CollectionWithPersistentOrder

So that we do not unnecessarily remove and re-add
sources and images if their order has been modified
in the updated style.
---
 src/mbgl/style/collection.hpp | 113 ++++++++++++++++++++++------------
 src/mbgl/style/style_impl.cpp |   1 +
 src/mbgl/style/style_impl.hpp |   4 +-
 3 files changed, 78 insertions(+), 40 deletions(-)

diff --git a/src/mbgl/style/collection.hpp b/src/mbgl/style/collection.hpp
index 0deb1411b..91244cb71 100644
--- a/src/mbgl/style/collection.hpp
+++ b/src/mbgl/style/collection.hpp
@@ -22,13 +22,13 @@ namespace style {
       * A `std::string getID() const` method
 */
 template <class T>
-class Collection {
+class CollectionBase {
 public:
     using Impl = typename T::Impl;
     using WrapperVector = std::vector<std::unique_ptr<T>>;
     using ImmutableVector = Immutable<std::vector<Immutable<Impl>>>;
 
-    Collection();
+    CollectionBase();
 
     std::size_t size() const;
     T* get(const std::string&) const;
@@ -41,46 +41,86 @@ public:
 
     void clear();
 
-    T* add(std::unique_ptr<T>, const optional<std::string>& = {});
-    std::unique_ptr<T> remove(const std::string&);
-
+protected:
+    std::size_t index(const std::string&) const;
+    T* add(std::size_t wrapperIndex, std::size_t implIndex, std::unique_ptr<T> wrapper);
+    std::unique_ptr<T> remove(std::size_t wrapperIndex, std::size_t implIndex);
     // Must be called whenever an element of the collection is internally mutated.
     // Typically, each element permits registration of an observer, and the observer
     // should call this method.
-    void update(const T&);
-
-private:
-    std::size_t index(const std::string&) const;
+    void update(std::size_t implIndex, const T&);
 
     WrapperVector wrappers;
     ImmutableVector impls;
 };
 
-template <class T>
-Collection<T>::Collection()
-    : impls(makeMutable<std::vector<Immutable<Impl>>>()) {
-}
+template <class T, bool persistentImplsOrder = false>
+class Collection;
 
 template <class T>
-std::size_t Collection<T>::size() const {
+class Collection<T, false /*persistentImplsOrder*/> : public CollectionBase<T> {
+    using Base = CollectionBase<T>;
+
+public:
+    T* add(std::unique_ptr<T> wrapper, const optional<std::string>& before = nullopt) {
+        std::size_t index = before ? Base::index(*before) : Base::size();
+        return Base::add(index, index, std::move(wrapper));
+    }
+    std::unique_ptr<T> remove(const std::string& id) {
+        std::size_t index = Base::index(id);
+        return Base::remove(index, index);
+    }
+    void update(const T& wrapper) { Base::update(Base::index(wrapper.getID()), wrapper); }
+};
+
+template <class T>
+class Collection<T, true /*persistentImplsOrder*/> : public CollectionBase<T> {
+    using Base = CollectionBase<T>;
+
+public:
+    T* add(std::unique_ptr<T> wrapper) {
+        std::size_t i = implsIndex(wrapper->getID());
+        return Base::add(Base::size(), i, std::move(wrapper));
+    }
+
+    std::unique_ptr<T> remove(const std::string& id) { return Base::remove(Base::index(id), implsIndex(id)); }
+
+    void update(const T& wrapper) { Base::update(implsIndex(wrapper.getID()), wrapper); }
+
+private:
+    std::size_t implsIndex(const std::string& id) const {
+        const auto& impls_ = *Base::impls;
+        auto it = std::lower_bound(
+            impls_.begin(), impls_.end(), id, [](const auto& a, const std::string& b) { return a->id < b; });
+        return it - impls_.begin();
+    }
+};
+template <class T>
+using CollectionWithPersistentOrder = Collection<T, true>;
+
+template <class T>
+CollectionBase<T>::CollectionBase() : impls(makeMutable<std::vector<Immutable<Impl>>>()) {}
+
+template <class T>
+std::size_t CollectionBase<T>::size() const {
     return wrappers.size();
 }
 
 template <class T>
-std::size_t Collection<T>::index(const std::string& id) const {
+std::size_t CollectionBase<T>::index(const std::string& id) const {
     return std::find_if(wrappers.begin(), wrappers.end(), [&](const auto& e) {
         return e->getID() == id;
     }) - wrappers.begin();
 }
 
 template <class T>
-T* Collection<T>::get(const std::string& id) const {
+T* CollectionBase<T>::get(const std::string& id) const {
     std::size_t i = index(id);
     return i < size() ? wrappers[i].get() : nullptr;
 }
 
 template <class T>
-std::vector<T*> Collection<T>::getWrappers() const {
+std::vector<T*> CollectionBase<T>::getWrappers() const {
     std::vector<T*> result;
     result.reserve(wrappers.size());
 
@@ -92,7 +132,7 @@ std::vector<T*> Collection<T>::getWrappers() const {
 }
 
 template <class T>
-void Collection<T>::clear() {
+void CollectionBase<T>::clear() {
     mutate(impls, [&] (auto& impls_) {
         impls_.clear();
     });
@@ -101,40 +141,37 @@ void Collection<T>::clear() {
 }
 
 template <class T>
-T* Collection<T>::add(std::unique_ptr<T> wrapper, const optional<std::string>& before) {
-    std::size_t i = before ? index(*before) : size();
+T* CollectionBase<T>::add(std::size_t wrapperIndex, std::size_t implIndex, std::unique_ptr<T> wrapper) {
+    assert(wrapperIndex <= size());
+    assert(implIndex <= size());
+    mutate(impls, [&](auto& impls_) { impls_.emplace(impls_.begin() + implIndex, wrapper->baseImpl); });
 
-    mutate(impls, [&] (auto& impls_) {
-        impls_.emplace(impls_.begin() + i, wrapper->baseImpl);
-    });
-
-    return wrappers.emplace(wrappers.begin() + i, std::move(wrapper))->get();
+    return wrappers.emplace(wrappers.begin() + wrapperIndex, std::move(wrapper))->get();
 }
 
 template <class T>
-std::unique_ptr<T> Collection<T>::remove(const std::string& id) {
-    std::size_t i = index(id);
-
-    if (i >= size()) {
+std::unique_ptr<T> CollectionBase<T>::remove(std::size_t wrapperIndex, std::size_t implIndex) {
+    if (wrapperIndex >= size()) {
         return nullptr;
     }
+    assert(implIndex < size());
 
-    auto source = std::move(wrappers[i]);
+    auto source = std::move(wrappers[wrapperIndex]);
 
-    mutate(impls, [&] (auto& impls_) {
-        impls_.erase(impls_.begin() + i);
-    });
+    mutate(impls, [&](auto& impls_) { impls_.erase(impls_.begin() + implIndex); });
 
-    wrappers.erase(wrappers.begin() + i);
+    wrappers.erase(wrappers.begin() + wrapperIndex);
 
     return source;
 }
 
 template <class T>
-void Collection<T>::update(const T& wrapper) {
-    mutate(impls, [&] (auto& impls_) {
-        impls_.at(this->index(wrapper.getID())) = wrapper.baseImpl;
-    });
+void CollectionBase<T>::update(std::size_t implIndex, const T& wrapper) {
+    if (implIndex >= size()) {
+        assert(false);
+        return;
+    }
+    mutate(impls, [&](auto& impls_) { impls_.at(implIndex) = wrapper.baseImpl; });
 }
 
 } // namespace style
diff --git a/src/mbgl/style/style_impl.cpp b/src/mbgl/style/style_impl.cpp
index 95a39819f..ef1f8436f 100644
--- a/src/mbgl/style/style_impl.cpp
+++ b/src/mbgl/style/style_impl.cpp
@@ -2,6 +2,7 @@
 #include <mbgl/storage/file_source.hpp>
 #include <mbgl/storage/resource.hpp>
 #include <mbgl/storage/response.hpp>
+#include <mbgl/style/image_impl.hpp>
 #include <mbgl/style/layer_impl.hpp>
 #include <mbgl/style/layers/background_layer.hpp>
 #include <mbgl/style/layers/circle_layer.hpp>
diff --git a/src/mbgl/style/style_impl.hpp b/src/mbgl/style/style_impl.hpp
index 4c56f6785..c4c0a9a41 100644
--- a/src/mbgl/style/style_impl.hpp
+++ b/src/mbgl/style/style_impl.hpp
@@ -106,8 +106,8 @@ private:
     std::unique_ptr<SpriteLoader> spriteLoader;
 
     std::string glyphURL;
-    Collection<style::Image> images;
-    Collection<Source> sources;
+    CollectionWithPersistentOrder<style::Image> images;
+    CollectionWithPersistentOrder<Source> sources;
     Collection<Layer> layers;
     TransitionOptions transitionOptions;
     std::unique_ptr<Light> light;