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.
This commit is contained in:
@@ -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);
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user