From d80d2d5eeed01b0ca2f0031ea26741c7da2f12fa Mon Sep 17 00:00:00 2001
From: "Thiago Marcos P. Santos" <tmpsantos@gmail.com>
Date: Fri, 14 Aug 2015 18:34:18 +0300
Subject: [PATCH] Remove request coalescing

Each NodeMap has its own NodeFileSource so we should not get multiple
requests of for the same resource anymore, making the coalescing
code path useless.

We still need to keep a list of pending requests indexed by Resources
for the same reason it was introduced before: Resources don't hold
a reference to a uv_loop_t, so it is fine to keep it around, unlike
mbgl::Requests. Otherwise it would cause deadlocks.

The mutex was also kept because the NodeFileSource is accessed by
some arbitrary thread created by mbgl::Map and also by the main
thread coming from the JavaScript engine. More precisely,
NodeFileSource::notify() is called from the main thread while
NodeFileSource::request() and NodeFileSource::cancel() are not,
but they all access the same data structure.
---
 src/node_file_source.cpp | 58 +++++++++++++---------------------------
 src/node_file_source.hpp | 12 ++++-----
 2 files changed, 23 insertions(+), 47 deletions(-)

diff --git a/src/node_file_source.cpp b/src/node_file_source.cpp
index 0ff4accdb..204e85a12 100644
--- a/src/node_file_source.cpp
+++ b/src/node_file_source.cpp
@@ -4,8 +4,6 @@
 
 #include <mbgl/storage/request.hpp>
 
-#include <algorithm>
-
 namespace node_mbgl {
 
 struct NodeFileSource::Action {
@@ -39,16 +37,13 @@ mbgl::Request* NodeFileSource::request(const mbgl::Resource& resource, uv_loop_t
     auto req = new mbgl::Request(resource, loop, std::move(callback));
 
     std::lock_guard<std::mutex> lock(observersMutex);
-    auto it = observers.find(resource);
-    if (it == observers.end()) {
-        observers[resource] = { req };
 
-        // This function can be called from any thread. Make sure we're executing the actual call in the
-        // file source loop by sending it over the queue. It will be processed in processAction().
-        queue->send(Action{ Action::Add, resource });
-    } else {
-        it->second.emplace_back(req);
-    }
+    assert(observers.find(resource) == observers.end());
+    observers[resource] = req;
+
+    // This function can be called from any thread. Make sure we're executing the actual call in the
+    // file source loop by sending it over the queue. It will be processed in processAction().
+    queue->send(Action{ Action::Add, resource });
 
     return req;
 }
@@ -58,25 +53,16 @@ void NodeFileSource::cancel(mbgl::Request* req) {
 
     std::lock_guard<std::mutex> lock(observersMutex);
 
-    auto observersIter = observers.find(req->resource);
-    if (observersIter == observers.end()) {
+    auto it = observers.find(req->resource);
+    if (it == observers.end()) {
         return;
     }
 
-    auto& observersList = observersIter->second;
-    auto observersListIter = std::find(observersList.begin(), observersList.end(), req);
-    if (observersListIter == observersList.end()) {
-        return;
-    }
+    observers.erase(it);
 
-    observersList.erase(observersListIter);
-    if (observersList.empty()) {
-        observers.erase(observersIter);
-
-        // This function can be called from any thread. Make sure we're executing the actual call in the
-        // file source loop by sending it over the queue. It will be processed in processAction().
-        queue->send(Action{ Action::Cancel, req->resource });
-    }
+    // This function can be called from any thread. Make sure we're executing the actual call in the
+    // file source loop by sending it over the queue. It will be processed in processAction().
+    queue->send(Action{ Action::Cancel, req->resource });
 
     req->destruct();
 }
@@ -161,23 +147,15 @@ void NodeFileSource::notify(const mbgl::Resource& resource, const std::shared_pt
         }
     }
 
-    std::vector<mbgl::Request*> observersList;
+    std::lock_guard<std::mutex> lock(observersMutex);
 
-    {
-        std::lock_guard<std::mutex> lock(observersMutex);
-
-        auto observersIter = observers.find(resource);
-        if (observersIter == observers.end()) {
-            return;
-        }
-
-        observersList.swap(observersIter->second);
-        observers.erase(observersIter);
+    auto observersIt = observers.find(resource);
+    if (observersIt == observers.end()) {
+        return;
     }
 
-    for (auto request : observersList) {
-        request->notify(response);
-    }
+    observersIt->second->notify(response);
+    observers.erase(observersIt);
 }
 
 }
diff --git a/src/node_file_source.hpp b/src/node_file_source.hpp
index 9dde87a2e..f412bdee1 100644
--- a/src/node_file_source.hpp
+++ b/src/node_file_source.hpp
@@ -13,7 +13,6 @@
 #include <memory>
 #include <mutex>
 #include <unordered_map>
-#include <vector>
 
 namespace node_mbgl {
 
@@ -47,13 +46,12 @@ private:
 #endif
 
     // The observers list will hold pointers to all the requests waiting
-    // for a particular resource. It is also used for coalescing requests,
-    // so we don't ask for the same resources twice. The access must be
-    // guarded by a mutex because the list is also accessed by a thread
-    // from the mbgl::Map object and from the main thread when notifying
-    // requests of completion. Concurrent access is specially needed when
+    // for a particular resource. The access must be guarded by a mutex
+    // because the list is also accessed by a thread from the mbgl::Map
+    // object and from the main thread when notifying requests of
+    // completion. Concurrent access is specially needed when
     // canceling a request to avoid a deadlock (see #129).
-    std::unordered_map<mbgl::Resource, std::vector<mbgl::Request*>, mbgl::Resource::Hash> observers;
+    std::unordered_map<mbgl::Resource, mbgl::Request*, mbgl::Resource::Hash> observers;
     std::mutex observersMutex;
 
     Queue *queue = nullptr;