From 50cf60b095e1ac991dea5b946171a86ff5ce04da Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 13 Feb 2020 15:29:37 +0200 Subject: [PATCH 1/5] Revert "[core] Fix excessive onSpriteLoaded() notifications" This reverts commit 80cb05420a86ed53815cae7fb2cb3fddf07dd1d1. --- src/mbgl/sprite/sprite_loader.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/mbgl/sprite/sprite_loader.cpp b/src/mbgl/sprite/sprite_loader.cpp index 6051de7ef4e..56f7abd50b6 100644 --- a/src/mbgl/sprite/sprite_loader.cpp +++ b/src/mbgl/sprite/sprite_loader.cpp @@ -58,9 +58,9 @@ void SpriteLoader::load(const std::string& url, FileSource& fileSource) { } else if (res.noContent) { loader->json = std::make_shared(); emitSpriteLoadedIfComplete(); - } else if (loader->json != res.data) { // They can be equal, see OnlineFileRequest::completed(). + } else { // Only trigger a sprite loaded event we got new data. - loader->json = std::move(res.data); + loader->json = res.data; emitSpriteLoadedIfComplete(); } }); @@ -73,8 +73,8 @@ void SpriteLoader::load(const std::string& url, FileSource& fileSource) { } else if (res.noContent) { loader->image = std::make_shared(); emitSpriteLoadedIfComplete(); - } else if (loader->image != res.data) { // They can be equal - see OnlineFileRequest::completed(). - loader->image = std::move(res.data); + } else { + loader->image = res.data; emitSpriteLoadedIfComplete(); } }); From d06ec7fbe0cd4fef7aa5b48404638c43d9e8af5c Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 13 Feb 2020 15:29:49 +0200 Subject: [PATCH 2/5] Revert "Add change log entry" This reverts commit d3535f1ca9f7c12b3c2290da3f347e4f95210425. --- CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bad7958a100..4bb86c8758f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,10 +32,6 @@ This change enables attaching images to the style with batches and avoids massive re-allocations. Thus, it improves UI performance especially at start-up time. - - [core] Fix excessive onSpriteLoaded() notifications ([#16196](https://github.com/mapbox/mapbox-gl-native/pull/16196)) - - The excessive `onSpriteLoaded()` notifications affected the render orchestration logic and could have significant negative performance impact. - ### 🧩 Architectural changes ##### ⚠️ Breaking changes From 3e6c161ac8b7a0fa2fda2c17f880fd1aa0e94f76 Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 13 Feb 2020 11:56:13 +0200 Subject: [PATCH 3/5] [core] Set priorData from cache only if resource is useable In cases when cached resource is useable, yet don't have an expiration timestamp, we provided data to the requester from the cache and the same data was returned once 304 response was received from the network. --- platform/default/src/mbgl/storage/main_resource_loader.cpp | 6 +++++- src/mbgl/sprite/sprite_loader.cpp | 6 ++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/platform/default/src/mbgl/storage/main_resource_loader.cpp b/platform/default/src/mbgl/storage/main_resource_loader.cpp index 9bdf09fa10f..aa8d745170a 100644 --- a/platform/default/src/mbgl/storage/main_resource_loader.cpp +++ b/platform/default/src/mbgl/storage/main_resource_loader.cpp @@ -81,13 +81,17 @@ class MainResourceLoaderThread { callback(response); // Set the priority of existing resource to low if it's expired but usable. res.setPriority(Resource::Priority::Low); + } else { + // Set prior data only if it was not returned to the requester. + // Once we get 304 response from the network, we will forward response + // to the requester. + res.priorData = response.data; } // Copy response fields for cache control request res.priorModified = response.modified; res.priorExpires = response.expires; res.priorEtag = response.etag; - res.priorData = response.data; } tasks[req] = requestFromNetwork(res, std::move(tasks[req])); diff --git a/src/mbgl/sprite/sprite_loader.cpp b/src/mbgl/sprite/sprite_loader.cpp index 56f7abd50b6..60bab2bf6eb 100644 --- a/src/mbgl/sprite/sprite_loader.cpp +++ b/src/mbgl/sprite/sprite_loader.cpp @@ -60,7 +60,8 @@ void SpriteLoader::load(const std::string& url, FileSource& fileSource) { emitSpriteLoadedIfComplete(); } else { // Only trigger a sprite loaded event we got new data. - loader->json = res.data; + assert(loader->json != res.data); + loader->json = std::move(res.data); emitSpriteLoadedIfComplete(); } }); @@ -74,7 +75,8 @@ void SpriteLoader::load(const std::string& url, FileSource& fileSource) { loader->image = std::make_shared(); emitSpriteLoadedIfComplete(); } else { - loader->image = res.data; + assert(loader->image != res.data); + loader->image = std::move(res.data); emitSpriteLoadedIfComplete(); } }); From 0ff2992fae3f93a6c6e36d1b4144a07b76604b3e Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 13 Feb 2020 14:00:57 +0200 Subject: [PATCH 4/5] [core] Add unit test --- test/storage/main_resource_loader.test.cpp | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/storage/main_resource_loader.test.cpp b/test/storage/main_resource_loader.test.cpp index ee9211b0645..b5245dbad89 100644 --- a/test/storage/main_resource_loader.test.cpp +++ b/test/storage/main_resource_loader.test.cpp @@ -8,6 +8,7 @@ #include #include #include +#include using namespace mbgl; @@ -749,3 +750,38 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(CachedResourceLowPriority)) { loop.run(); } + +TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoDoubleDispatch)) { + util::RunLoop loop; + MainResourceLoader fs(ResourceOptions{}); + + const Resource resource{Resource::Unknown, "http://127.0.0.1:3000/revalidate-same"}; + Response response; + response.data = std::make_shared("data"); + response.etag.emplace("snowfall"); + + std::unique_ptr req; + unsigned responseCount = 0u; + auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + dbfs->forward(resource, response, [&] { + req = fs.request(resource, [&](Response res) { + EXPECT_EQ(nullptr, res.error); + EXPECT_FALSE(bool(res.modified)); + EXPECT_TRUE(bool(res.etag)); + EXPECT_EQ("snowfall", *res.etag); + if (!res.notModified) { + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("data", *res.data); + ++responseCount; + } + }); + }); + + util::Timer timer; + timer.start(Milliseconds(100), Duration::zero(), [&loop, &responseCount] { + EXPECT_EQ(1u, responseCount); + loop.stop(); + }); + + loop.run(); +} From 0cb54962fb0e02d294570fe78dbe3bd846635422 Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 13 Feb 2020 16:28:11 +0200 Subject: [PATCH 5/5] [core] Update changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bb86c8758f..cf0a476107b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,12 @@ - [core] Add support for using `within expression` with layout propery. ([#16194](https://github.com/mapbox/mapbox-gl-native/pull/16194)) +### 🐞 Bug fixes + +- [core] Don't provide multiple responses with the same data for 304 replies ([#16200](https://github.com/mapbox/mapbox-gl-native/pull/16200)) + + In cases when cached resource is useable, yet don't have an expiration timestamp, we provided data to the requester from the cache and the same data was returned once 304 response was received from the network. + ### 🏁 Performance improvements - [core] Loading images to style optimization ([#16187](https://github.com/mapbox/mapbox-gl-native/pull/16187))