Merge lp:~michihenning/thumbnailer/fix-1531038 into lp:thumbnailer/devel

Proposed by Michi Henning on 2016-01-07
Status: Merged
Approved by: Michi Henning on 2016-01-07
Approved revision: 328
Merged at revision: 328
Proposed branch: lp:~michihenning/thumbnailer/fix-1531038
Merge into: lp:thumbnailer/devel
Diff against target: 109 lines (+11/-12)
5 files modified
data/com.canonical.Unity.Thumbnailer.gschema.xml (+1/-1)
include/ratelimiter.h (+2/-2)
man/thumbnailer-settings.5 (+1/-1)
src/ratelimiter.cpp (+5/-6)
tests/settings/settings_test.cpp (+2/-2)
To merge this branch: bzr merge lp:~michihenning/thumbnailer/fix-1531038
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve on 2016-01-07
Michi Henning (community) Approve on 2016-01-07
Review via email: mp+281813@code.launchpad.net

Commit message

Changed default backlog to 10. Change RateLimiter to work on LIFO basis. Fixes lp:1531038

Description of the change

Changed default backlog to 10. This is necessary because, with a backlog of 20, we ended up never cancelling anything because the sliding window around a list view is smaller than this. In turn, this meant that we always ended up having fewer outstanding requests than the limit, so the cancel messages from the list view always arrived after the request had already gone out on the wire.

Changed the RateLimiter to work on a LIFO basis instead of FIFO. By dealing with the most request first, cancellation has a much better chance of actually cancelling something. In addition, this massively improves the user experience when scrolling through a large collection with a cold cache. Previously, when scrolling down quickly for, say, 150 songs) all the requests for songs that were scrolled over and disappeared off the top of the screen had to complete before the thumbnails for visible region (once scrolling stopped) became visible. With this change, we give priority to the most recent request, so the thumbnails for the visible region where scrolling stops fill in quickly.

To post a comment you must log in.
Michi Henning (michihenning) wrote :

Top-approving after lots of testing on the phone.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/com.canonical.Unity.Thumbnailer.gschema.xml'
2--- data/com.canonical.Unity.Thumbnailer.gschema.xml 2015-12-30 01:24:33 +0000
3+++ data/com.canonical.Unity.Thumbnailer.gschema.xml 2016-01-07 03:18:37 +0000
4@@ -74,7 +74,7 @@
5 </key>
6
7 <key type="i" name="max-backlog">
8- <default>20</default>
9+ <default>10</default>
10 <summary>Maximum number of pending DBus requests before the thumbnailer starts queuing them.</summary>
11 <description>
12 This parameter limits the number of pending DBus requests to the thumbnailer service. If there are more requests than this currently in progress, additional requests are queued and sent once the backlog drops below the limit.
13
14=== modified file 'include/ratelimiter.h'
15--- include/ratelimiter.h 2015-12-21 01:18:40 +0000
16+++ include/ratelimiter.h 2016-01-07 03:18:37 +0000
17@@ -21,7 +21,7 @@
18
19 #include <functional>
20 #include <memory>
21-#include <queue>
22+#include <list>
23
24 namespace unity
25 {
26@@ -67,7 +67,7 @@
27 int running_; // Actual number of outstanding requests.
28 // We store a shared_ptr so we can detect on cancellation
29 // whether a job completed before it was cancelled.
30- std::queue<std::shared_ptr<std::function<void()>>> queue_;
31+ std::list<std::shared_ptr<std::function<void()>>> list_;
32 };
33
34 } // namespace thumbnailer
35
36=== modified file 'man/thumbnailer-settings.5'
37--- man/thumbnailer-settings.5 2016-01-04 06:09:41 +0000
38+++ man/thumbnailer-settings.5 2016-01-07 03:18:37 +0000
39@@ -59,7 +59,7 @@
40 .TP
41 .B max\-backlog \fR(int)\fP
42 Controls the number of DBus requests that will be sent before queueing the requests internally.
43-The default is 20 requests.
44+The default is 10 requests.
45 .TP
46 .B trace\-client \fR(bool)\fP
47 If true, thumbnail and cancel requests are logged. Log messages are written to the calling application's log
48
49=== modified file 'src/ratelimiter.cpp'
50--- src/ratelimiter.cpp 2015-12-21 01:18:40 +0000
51+++ src/ratelimiter.cpp 2016-01-07 03:18:37 +0000
52@@ -48,18 +48,17 @@
53 {
54 assert(job);
55 assert (running_ >= 0);
56- assert (running_ <= concurrency_);
57
58 if (running_ < concurrency_)
59 {
60 return schedule_now(job);
61 }
62
63- queue_.emplace(make_shared<function<void()>>(move(job)));
64+ list_.emplace_back(make_shared<function<void()>>(move(job)));
65
66 // Returned function clears the job when called, provided the job is still in the queue.
67 // done() removes any cleared jobs from the queue without calling them.
68- weak_ptr<function<void()>> weak_p(queue_.back());
69+ weak_ptr<function<void()>> weak_p(list_.back());
70 return [this, weak_p]() noexcept
71 {
72 auto job_p = weak_p.lock();
73@@ -87,11 +86,11 @@
74
75 // Find the next job, discarding any cancelled jobs.
76 shared_ptr<function<void()>> job_p;
77- while (!queue_.empty())
78+ while (!list_.empty())
79 {
80- job_p = queue_.front();
81+ job_p = list_.back();
82 assert(job_p);
83- queue_.pop();
84+ list_.pop_back();
85 if (*job_p != nullptr)
86 {
87 break;
88
89=== modified file 'tests/settings/settings_test.cpp'
90--- tests/settings/settings_test.cpp 2016-01-04 06:09:41 +0000
91+++ tests/settings/settings_test.cpp 2016-01-07 03:18:37 +0000
92@@ -47,7 +47,7 @@
93 EXPECT_EQ(8, settings.max_downloads());
94 EXPECT_EQ(0, settings.max_extractions());
95 EXPECT_EQ(10, settings.extraction_timeout());
96- EXPECT_EQ(20, settings.max_backlog());
97+ EXPECT_EQ(10, settings.max_backlog());
98 EXPECT_FALSE(settings.trace_client());
99 EXPECT_EQ(1, settings.log_level());
100 }
101@@ -70,7 +70,7 @@
102 EXPECT_EQ(8, settings.max_downloads());
103 EXPECT_EQ(0, settings.max_extractions());
104 EXPECT_EQ(10, settings.extraction_timeout());
105- EXPECT_EQ(20, settings.max_backlog());
106+ EXPECT_EQ(10, settings.max_backlog());
107 EXPECT_FALSE(settings.trace_client());
108 EXPECT_EQ(1, settings.log_level());
109 }

Subscribers

People subscribed via source and target branches

to all changes: