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

[Michi Henning]

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

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.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:328
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/623/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/327
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/328
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/328/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/328
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/77
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/78
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/78/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/75

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/623/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:329
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/624/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/328/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/329/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/329/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/78/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/79/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/76/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/624/rebuild

review: Needs Fixing (continuous-integration)

[Michi Henning (michihenning)]

Top-approving after lots of testing on the phone.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:328
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/625/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/329
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/330
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/330/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/330
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/79
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/80
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/80/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/77

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/625/rebuild

review: Approve (continuous-integration)

=== modified file 'data/com.canonical.Unity.Thumbnailer.gschema.xml'
--- data/com.canonical.Unity.Thumbnailer.gschema.xml	2015-12-30 01:24:33 +0000
+++ data/com.canonical.Unity.Thumbnailer.gschema.xml	2016-01-07 03:18:37 +0000
@@ -74,7 +74,7 @@
     </key>
 
     <key type="i" name="max-backlog">
-      <default>20</default>
+      <default>10</default>
       <summary>Maximum number of pending DBus requests before the thumbnailer starts queuing them.</summary>
       <description>
         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.

=== modified file 'include/ratelimiter.h'
--- include/ratelimiter.h	2015-12-21 01:18:40 +0000
+++ include/ratelimiter.h	2016-01-07 03:18:37 +0000
@@ -21,7 +21,7 @@
 
 #include <functional>
 #include <memory>
-#include <queue>
+#include <list>
 
 namespace unity
 {
@@ -67,7 +67,7 @@
     int running_;            // Actual number of outstanding requests.
     // We store a shared_ptr so we can detect on cancellation
     // whether a job completed before it was cancelled.
-    std::queue<std::shared_ptr<std::function<void()>>> queue_;
+    std::list<std::shared_ptr<std::function<void()>>> list_;
 };
 
 }  // namespace thumbnailer

=== modified file 'man/thumbnailer-settings.5'
--- man/thumbnailer-settings.5	2016-01-04 06:09:41 +0000
+++ man/thumbnailer-settings.5	2016-01-07 03:18:37 +0000
@@ -59,7 +59,7 @@
 .TP
 .B max\-backlog \fR(int)\fP
 Controls the number of DBus requests that will be sent before queueing the requests internally.
-The default is 20 requests.
+The default is 10 requests.
 .TP
 .B trace\-client \fR(bool)\fP
 If true, thumbnail and cancel requests are logged. Log messages are written to the calling application's log

=== modified file 'src/ratelimiter.cpp'
--- src/ratelimiter.cpp	2015-12-21 01:18:40 +0000
+++ src/ratelimiter.cpp	2016-01-07 03:18:37 +0000
@@ -48,18 +48,17 @@
 {
     assert(job);
     assert (running_ >= 0);
-    assert (running_ <= concurrency_);
 
     if (running_ < concurrency_)
     {
         return schedule_now(job);
     }
 
-    queue_.emplace(make_shared<function<void()>>(move(job)));
+    list_.emplace_back(make_shared<function<void()>>(move(job)));
 
     // Returned function clears the job when called, provided the job is still in the queue.
     // done() removes any cleared jobs from the queue without calling them.
-    weak_ptr<function<void()>> weak_p(queue_.back());
+    weak_ptr<function<void()>> weak_p(list_.back());
     return [this, weak_p]() noexcept
     {
         auto job_p = weak_p.lock();
@@ -87,11 +86,11 @@
 
     // Find the next job, discarding any cancelled jobs.
     shared_ptr<function<void()>> job_p;
-    while (!queue_.empty())
+    while (!list_.empty())
     {
-        job_p = queue_.front();
+        job_p = list_.back();
         assert(job_p);
-        queue_.pop();
+        list_.pop_back();
         if (*job_p != nullptr)
         {
             break;

=== modified file 'tests/settings/settings_test.cpp'
--- tests/settings/settings_test.cpp	2016-01-04 06:09:41 +0000
+++ tests/settings/settings_test.cpp	2016-01-07 03:18:37 +0000
@@ -47,7 +47,7 @@
     EXPECT_EQ(8, settings.max_downloads());
     EXPECT_EQ(0, settings.max_extractions());
     EXPECT_EQ(10, settings.extraction_timeout());
-    EXPECT_EQ(20, settings.max_backlog());
+    EXPECT_EQ(10, settings.max_backlog());
     EXPECT_FALSE(settings.trace_client());
     EXPECT_EQ(1, settings.log_level());
 }
@@ -70,7 +70,7 @@
     EXPECT_EQ(8, settings.max_downloads());
     EXPECT_EQ(0, settings.max_extractions());
     EXPECT_EQ(10, settings.extraction_timeout());
-    EXPECT_EQ(20, settings.max_backlog());
+    EXPECT_EQ(10, settings.max_backlog());
     EXPECT_FALSE(settings.trace_client());
     EXPECT_EQ(1, settings.log_level());
 }