[core] Revert coalescing requests to the client for the same missing image

This commit is contained in:
Mikhail Pozdnyakov
2019-10-24 13:13:01 +03:00
parent 4fb75ba32c
commit 424cf20207
3 changed files with 33 additions and 79 deletions

View File

@@ -138,9 +138,8 @@ void ImageManager::removeRequestor(ImageRequestor& requestor) {
void ImageManager::notifyIfMissingImageAdded() {
for (auto it = missingImageRequestors.begin(); it != missingImageRequestors.end();) {
ImageRequestor& requestor = *it->first;
if (!requestor.hasPendingRequests()) {
notify(requestor, it->second);
if (it->second.callbacks.empty()) {
notify(*it->first, it->second.pair);
it = missingImageRequestors.erase(it);
} else {
++it;
@@ -170,56 +169,38 @@ void ImageManager::reduceMemoryUseIfCacheSizeExceedsLimit() {
}
void ImageManager::checkMissingAndNotify(ImageRequestor& requestor, const ImageRequestPair& pair) {
ImageDependencies missingDependencies;
std::vector<std::string> missingImages;
missingImages.reserve(pair.first.size());
for (const auto& dependency : pair.first) {
if (images.find(dependency.first) == images.end()) {
missingDependencies.emplace(dependency);
missingImages.push_back(dependency.first);
}
}
if (!missingDependencies.empty()) {
if (!missingImages.empty()) {
ImageRequestor* requestorPtr = &requestor;
assert(!missingImageRequestors.count(requestorPtr));
missingImageRequestors.emplace(requestorPtr, pair);
for (const auto& dependency : missingDependencies) {
const std::string& missingImage = dependency.first;
auto emplaced = missingImageRequestors.emplace(requestorPtr, MissingImageRequestPair { pair, {} });
assert(emplaced.second);
for (const auto& missingImage : missingImages) {
assert(observer != nullptr);
requestedImages[missingImage].emplace(&requestor);
auto callback = std::make_unique<ActorCallback>(
*Scheduler::GetCurrent(),
[this, requestorPtr, missingImage] {
auto requestorIt = missingImageRequestors.find(requestorPtr);
if (requestorIt != missingImageRequestors.end()) {
assert(requestorIt->second.callbacks.find(missingImage) != requestorIt->second.callbacks.end());
requestorIt->second.callbacks.erase(missingImage);
}
});
auto existingRequestorsIt = requestedImages.find(missingImage);
if (existingRequestorsIt != requestedImages.end()) { // Already asked client about this image.
std::set<ImageRequestor*>& existingRequestors = existingRequestorsIt->second;
if (!existingRequestors.empty() &&
(*existingRequestors.begin())
->hasPendingRequest(missingImage)) { // Still waiting for the client response for this image.
requestorPtr->addPendingRequest(missingImage);
existingRequestors.emplace(requestorPtr);
continue;
}
// Unlike icons, pattern changes are not caught
// with style-diff meaning that the existing request
// could be from the previous style and we cannot
// coalesce requests for them.
if (dependency.second != ImageType::Pattern) {
continue;
}
}
requestedImages[missingImage].emplace(requestorPtr);
requestor.addPendingRequest(missingImage);
auto removePendingRequests = [this, missingImage] {
auto existingRequest = requestedImages.find(missingImage);
if (existingRequest == requestedImages.end()) {
return;
}
for (auto* req : existingRequest->second) {
req->removePendingRequest(missingImage);
}
};
observer->onStyleImageMissing(missingImage,
Scheduler::GetCurrent()->bindOnce(std::move(removePendingRequests)));
auto actorRef = callback->self();
emplaced.first->second.callbacks.emplace(missingImage, std::move(callback));
observer->onStyleImageMissing(missingImage, [actorRef] {
actorRef.invoke(&Callback::operator());
});
}
} else {
// Associate requestor with an image that was provided by the client.
@@ -249,7 +230,7 @@ void ImageManager::notify(ImageRequestor& requestor, const ImageRequestPair& pai
}
}
requestor.onImagesAvailable(std::move(iconMap), std::move(patternMap), std::move(versionMap), pair.second);
requestor.onImagesAvailable(iconMap, patternMap, std::move(versionMap), pair.second);
}
void ImageManager::dumpDebugLogs() const {

View File

@@ -58,7 +58,13 @@ private:
bool loaded = false;
std::map<ImageRequestor*, ImageRequestPair> requestors;
std::map<ImageRequestor*, ImageRequestPair> missingImageRequestors;
using Callback = std::function<void()>;
using ActorCallback = Actor<Callback>;
struct MissingImageRequestPair {
ImageRequestPair pair;
std::map<std::string, std::unique_ptr<ActorCallback>> callbacks;
};
std::map<ImageRequestor*, MissingImageRequestPair> missingImageRequestors;
std::map<std::string, std::set<ImageRequestor*>> requestedImages;
std::size_t requestedImagesCacheSize = 0ul;
ImageMap images;
@@ -71,17 +77,8 @@ public:
explicit ImageRequestor(ImageManager&);
virtual ~ImageRequestor();
virtual void onImagesAvailable(ImageMap icons, ImageMap patterns, ImageVersionMap versionMap, uint64_t imageCorrelationID) = 0;
void addPendingRequest(const std::string& imageId) { pendingRequests.insert(imageId); }
bool hasPendingRequest(const std::string& imageId) const { return pendingRequests.count(imageId); }
bool hasPendingRequests() const { return !pendingRequests.empty(); }
void removePendingRequest(const std::string& imageId) { pendingRequests.erase(imageId); }
private:
ImageManager& imageManager;
// Pending requests are image requests that are waiting to be dispatched to the client.
std::set<std::string> pendingRequests;
};
} // namespace mbgl

View File

@@ -187,30 +187,6 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) {
EXPECT_EQ(observer.count, 1);
ASSERT_TRUE(notified);
// Repeated request of the same image shall not result another
// `ImageManagerObserver.onStyleImageMissing()` call.
imageManager.getImages(requestor, std::make_pair(dependencies, ++imageCorrelationID));
runLoop.runOnce();
EXPECT_EQ(observer.count, 1);
// Request for updated dependencies must be dispatched to the
// observer.
dependencies.emplace("post", ImageType::Icon);
imageManager.getImages(requestor, std::make_pair(dependencies, ++imageCorrelationID));
runLoop.runOnce();
EXPECT_EQ(observer.count, 2);
// Another requestor shall not have pending requests for already obtained images.
StubImageRequestor anotherRequestor(imageManager);
imageManager.getImages(anotherRequestor, std::make_pair(dependencies, ++imageCorrelationID));
ASSERT_FALSE(anotherRequestor.hasPendingRequests());
dependencies.emplace("unfamiliar", ImageType::Icon);
imageManager.getImages(anotherRequestor, std::make_pair(dependencies, ++imageCorrelationID));
EXPECT_TRUE(anotherRequestor.hasPendingRequests());
EXPECT_TRUE(anotherRequestor.hasPendingRequest("unfamiliar"));
}
TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) {