[Merged][core] Deprioritize offline downloads #13019

chaupow · 2018-10-03T15:38:43Z

Working towards targeting #12655:

This issue covers prioritizing all regular requests, i.e. all requests necessary to show the current map view, above offline download request. This should resolve the issue outlined in #11461.

Current approach:

Add a priority-aware logic to favour "regular" requests over offline requests
- currently implemented by adding two queues in OnlineFileSource
Set requests made by OfflineDownload to low priority
- currently realised by adding a bool isLowPriority flag to Resource

Todos:

implement current approach
get feedback on approach
- currently, offline requests set their resources to low priority by calling setLowPriority() here. Would it be better to set the priority flag via an input parameter in the constructors? Should it be a parameter that defaults to zero? Or would it be better to enforce it being set explicitly?
test whether this is actually working as intended
check manually if there's an improvement by downloading data and scrolling in the app
clean up code
- rename commit messages to precede with [core]
get a code review

related work: #12910

kkaefer · 2018-10-08T08:55:59Z

include/mbgl/storage/resource.hpp

    Kind kind;
    LoadingMethod loadingMethod;
    std::string url;
+    std::size_t isLowPriority;


Using a boolean or enum would better convey the fact that this is a flag, and not a scalar type.

kkaefer · 2018-10-08T09:01:10Z

platform/default/online_file_source.cpp

+        // context: https://github.com/mapbox/mapbox-gl-native/issues/12655
+        PendingRequests() : prioritizedPendingRequestsList(2), prioritizedPendingRequestsMap(2) {}
+
+        std::vector<std::list<OnlineFileRequest*>> prioritizedPendingRequestsList;


Using std::array<std::list<OnlineFileRequest*>, 2> conveys the intention better: a std::vector is resizable and uses dynamically allocated memory, while a std::array is fixed size and is stored inline.

kkaefer · 2018-10-08T09:02:15Z

platform/default/online_file_source.cpp

+                it = prioritizedPendingRequestsMap[1].find(request);
+                if (it != prioritizedPendingRequestsMap[1].end()) {
+                    prioritizedPendingRequestsList[1].erase(it->second);
+                    prioritizedPendingRequestsMap[1].erase(it);


To reduce code duplication, you could wrap the list + map together in one class, and handle the prioritization outside of the class.

platform/default/online_file_source.cpp

kkaefer · 2018-10-08T09:14:45Z

platform/default/online_file_source.cpp

+        PendingRequests() : prioritizedPendingRequestsList(2), prioritizedPendingRequestsMap(2) {}
+
+        std::vector<std::list<OnlineFileRequest*>> prioritizedPendingRequestsList;
+        std::vector<std::unordered_map<OnlineFileRequest*, std::list<OnlineFileRequest*>::iterator>> prioritizedPendingRequestsMap;


The original code used a list and a map with request pointer -> list iterator and was added in f5b521f, but I'm not convinced that we need two separate data structures here. Given that the number of requests we have in the queue is usually pretty small (<100), a linear search in the queue might be just as fast and would simplify the logic quite a bit.

asheemmamoowala · 2018-10-12T18:33:50Z

platform/default/online_file_source.cpp

+            else {
+                assert(num_high_priority_requests <= (int) queue.size());
+                auto new_pos = queue.begin();
+                for (int i = 0; i < num_high_priority_requests; i++) new_pos++;


Instead of this iterator increment - you could replace num_high_priority_requests with the iterator to the last inserted high priority request. Regular insertions can then be made without traversing the list. When removing/popping requests, an additional check is needed before the removal/pop to ensure that the lastHighPriorityRequest iterator points to a valid list entry.

test/storage/online_file_source.test.cpp

asheemmamoowala · 2018-10-12T18:51:54Z

include/mbgl/storage/resource.hpp

          tileData(std::move(tileData_)) {
    }

+    void setLowPriority() { isLowPriority = true; }


Would an enum with regular and low/background be more appropriate here?

chaupow · 2018-10-18T12:06:46Z

Tests pass, I have cleaned up the commit history and it is rebased to current master.

@kkaefer or @asheemmamoowala mind taking another look? 🙂

Wrt to the needed changes for setting the number of maximum concurrent requests, I have opened another separate issue here #13130 and was thinking on working on that next as a follow-up issue of this PR.

asheemmamoowala

Approved pending a few nit picks

platform/default/online_file_source.cpp

asheemmamoowala · 2018-10-18T21:53:08Z

platform/default/online_file_source.cpp

@@ -169,13 +156,80 @@ class OnlineFileSource::Impl {
        networkIsReachableAgain();
    }

+    void setMaximumConcurrentRequestsOverride(const uint32_t maximum_concurrent_requests_override_) {
+        maximum_concurrent_requests_override = maximum_concurrent_requests_override_;


nit: use camelCase for variable names

asheemmamoowala · 2018-10-18T21:53:46Z

platform/default/online_file_source.cpp

    void networkIsReachableAgain() {
        for (auto& request : allRequests) {
            request->networkIsReachableAgain();
        }
    }

+
+    struct PendingRequests {
+        PendingRequests() : queue(), first_low(queue.begin()) {}


nit: nit: use camelCase for variable names. Also expand first_low to firstLowPriorityRequest

kkaefer

How are we handling requests that get added to the queue with a low priority, then another request comes in that has standard priority? Are they getting upgraded?

kkaefer · 2018-10-19T08:25:53Z

include/mbgl/storage/resource.hpp

@@ -24,6 +24,11 @@ class Resource {
        Image
    };

+    enum Priority : uint8_t {


Let's use an enum class here. Ideally the type would be bool

chaupow · 2018-10-22T12:46:58Z

Changed some small fixups according to the comments 🙂

Gonna merge ~~now~~ as soon as GH is back 🎉 Thanks for helping pushing it through, everyone! ❤️✨

How are we handling requests that get added to the queue with a low priority, then another request comes in that has standard priority? Are they getting upgraded?

Currently, the requests would not be upgraded but appear in the queue twice. If we request a resource A and there is a request A1 with low priority and we request it a second time with a regular priority in request A2, both will be in the queue, A2 will be processed first and then we will process A1

As far I understand, there is no deduplication logic in online_file_source. The default_file_source has some logic to not request resources if they're already in the offline database here but as soon as the requests are in the queue, there is no deduplication (please correct me if wrong).

This is something that can be checked before we insert a request in a queue but the benefit is probably not too high to do it now.

- priorities can be low or regular - offline downloads should have low priority to not throttle "regular requests"

- only process low priority requests (=offline requests) if there are no outstanding regular requests - favour regular requests over low priority ones

- ensure that low priority requests are handled last - add option to set the number of maximum concurrent requests for tests - some style fixups

chaupow force-pushed the deprioritize_offline_downloads branch 5 times, most recently from 719c4da to 843b8b5 Compare October 5, 2018 09:14

friedbunny assigned chaupow Oct 5, 2018

friedbunny added feature performance Speed, stability, CPU usage, memory usage, or power usage offline Core The cross-platform C++ core, aka mbgl labels Oct 5, 2018

kkaefer reviewed Oct 8, 2018

View reviewed changes

asheemmamoowala reviewed Oct 12, 2018

View reviewed changes

chaupow force-pushed the deprioritize_offline_downloads branch 11 times, most recently from ebb0026 to c2a949c Compare October 18, 2018 08:51

chaupow mentioned this pull request Oct 18, 2018

Refactor the number of concurrent http requests when downloading files #13130

Closed

chaupow changed the title ~~[WIP][core] Deprioritize offline downloads~~ [Waiting for review][core] Deprioritize offline downloads Oct 18, 2018

asheemmamoowala approved these changes Oct 18, 2018

View reviewed changes

kkaefer reviewed Oct 19, 2018

View reviewed changes

chaupow force-pushed the deprioritize_offline_downloads branch 2 times, most recently from 969cd07 to 25f659a Compare October 19, 2018 12:55

chaupow added 3 commits October 22, 2018 15:02

[core] add priorities to resources

9783a08

- priorities can be low or regular - offline downloads should have low priority to not throttle "regular requests"

[core] add logic of high and low priority requests to OnlineFileSource

dddef19

- only process low priority requests (=offline requests) if there are no outstanding regular requests - favour regular requests over low priority ones

[core] add tests for handling requests with different priorities

eedf8cf

- ensure that low priority requests are handled last - add option to set the number of maximum concurrent requests for tests - some style fixups

chaupow force-pushed the deprioritize_offline_downloads branch from 25f659a to eedf8cf Compare October 22, 2018 13:03

chaupow merged commit fa78bca into master Oct 23, 2018

chaupow deleted the deprioritize_offline_downloads branch October 23, 2018 10:47

chaupow changed the title ~~[Waiting for review][core] Deprioritize offline downloads~~ [Merged][core] Deprioritize offline downloads Oct 23, 2018

friedbunny added the needs changelog Indicates PR needs a changelog entry prior to merging. label Oct 23, 2018

friedbunny added this to the release-h milestone Oct 24, 2018

This was referenced Oct 24, 2018

Prioritise regular requests above offline download requests #12655

Closed

[ios] Updates for 4.6.0-alpha.2 release #13181

Merged

chaupow mentioned this pull request Oct 25, 2018

[macos] Add changelog for PR 13019 #13191

Merged

1 task

friedbunny removed needs changelog Indicates PR needs a changelog entry prior to merging. labels Oct 31, 2018

chaupow mentioned this pull request Nov 1, 2018

[core] Refactor the number of concurrent http requests when downloading files #13189

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged][core] Deprioritize offline downloads #13019

[Merged][core] Deprioritize offline downloads #13019

chaupow commented Oct 3, 2018 •

edited

Loading

kkaefer Oct 8, 2018

kkaefer Oct 8, 2018

kkaefer Oct 8, 2018

kkaefer Oct 8, 2018

asheemmamoowala Oct 12, 2018

asheemmamoowala Oct 12, 2018

chaupow commented Oct 18, 2018

asheemmamoowala left a comment

asheemmamoowala Oct 18, 2018

asheemmamoowala Oct 18, 2018

kkaefer left a comment

kkaefer Oct 19, 2018

chaupow commented Oct 22, 2018 •

edited

Loading

[Merged][core] Deprioritize offline downloads #13019

[Merged][core] Deprioritize offline downloads #13019

Conversation

chaupow commented Oct 3, 2018 • edited Loading

Current approach:

Todos:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaupow commented Oct 18, 2018

asheemmamoowala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaupow commented Oct 22, 2018 • edited Loading

chaupow commented Oct 3, 2018 •

edited

Loading

chaupow commented Oct 22, 2018 •

edited

Loading