From 424cf2020700f4e8ff4cef6906c5ed85674a1250 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Thu, 24 Oct 2019 13:13:01 +0300 Subject: [core] Revert coalescing requests to the client for the same missing image --- src/mbgl/renderer/image_manager.cpp | 73 +++++++++++++----------------------- src/mbgl/renderer/image_manager.hpp | 17 ++++----- test/renderer/image_manager.test.cpp | 24 ------------ 3 files changed, 34 insertions(+), 80 deletions(-) diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp index 7b51fb9a2e..d001084f92 100644 --- a/src/mbgl/renderer/image_manager.cpp +++ b/src/mbgl/renderer/image_manager.cpp @@ -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 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; - assert(observer != nullptr); + auto emplaced = missingImageRequestors.emplace(requestorPtr, MissingImageRequestPair { pair, {} }); + assert(emplaced.second); - auto existingRequestorsIt = requestedImages.find(missingImage); - if (existingRequestorsIt != requestedImages.end()) { // Already asked client about this image. - std::set& 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))); + for (const auto& missingImage : missingImages) { + assert(observer != nullptr); + requestedImages[missingImage].emplace(&requestor); + auto callback = std::make_unique( + *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 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 { diff --git a/src/mbgl/renderer/image_manager.hpp b/src/mbgl/renderer/image_manager.hpp index 5ed6e237f0..9097418681 100644 --- a/src/mbgl/renderer/image_manager.hpp +++ b/src/mbgl/renderer/image_manager.hpp @@ -58,7 +58,13 @@ private: bool loaded = false; std::map requestors; - std::map missingImageRequestors; + using Callback = std::function; + using ActorCallback = Actor; + struct MissingImageRequestPair { + ImageRequestPair pair; + std::map> callbacks; + }; + std::map missingImageRequestors; std::map> 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 pendingRequests; }; } // namespace mbgl diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp index 20c0a3a7f3..16700d713f 100644 --- a/test/renderer/image_manager.test.cpp +++ b/test/renderer/image_manager.test.cpp @@ -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) { -- cgit v1.2.1