Merge lp:~adamreichold/qpdfview/track-exposed-tiles into lp:qpdfview

[Adam Reichold]

After noticing that the interface begins to "stutter" using high scale factors (roughly above 1000%) and isolating this to using tiling and continuous layout, I noticed during profiling that cancellation has a pretty large overhead. This was somewhat reduced by making sure that the atomic operations to set cancellation were inlined, but even then the atomic operations to cancel and to reset instances of "QPixmap" are still noticable.

It seems the main cause of this is the document view calling "PageItem::cancelRender" on all not-visible page items and with a high scale factor implying a large number of tiles (e.g. 500 tiles per page at 1200%), this easily yields 100000 tiles being cancelled even though rendering on them was not active at all.

This branch tries to alleviate the issue by making the page item explicitly track the exposed tiles and letting the document view cancel only those (for non-force cancellation, i.e. without interrupting prefetching which in itself is a problem with a high tile count) that were actually exposed. The page item itself will also cancel tiles when and only when they are removed from the set of exposed tiles which again reduces the number of pointless cancellation attempts.

The necessary changes are pretty limited, but as they touch a very fundamental function of the program, I would be grateful for a review and test by another developer.

[lp:~adamreichold/qpdfview/track-exposed-tiles]

1858. By Adam Reichold on 2015-02-01

Also clear set of exposed tiles after cancellation.

1859. By Adam Reichold on 2015-02-01

Remerge with trunk to suppress unrelated change.

[Adam Reichold (adamreichold)]

Maybe we should also consider making "TileItem::cancelRender" inline since even after these optimizations, it shows up on top of gprof's list. I strongly dislike this since it exposes the "RenderTask" internals of "TileItem", but then again "tileitem.h" (and hence indirectly "rendertask.h") are included into "tileitem.cpp" and "pageitem.cpp" only.

It seems like a sensible and pratically free optimization, but it also does not seem to noticeably reduce the "stutter" in any way, so I am very much unsure about it.

[Razi Alavizadeh (srazi)]

Your patch seems OK. but I didn't see a big difference here. I think maybe the solution is using a limit for number of "tile"s? Because when testing this branch I found that when using a high scale e.g. 1200% "qpdfview.exe" uses more than 120,000 handles and if I change scale factor to 1933% there will be more than 300,000 handles used by qpdfview.exe

[Adam Reichold (adamreichold)]

Hello,

that's strange as on my system this patch made the difference between stuttering and being able to set that we might need better double-buffering as well. :-\

When you say handles, I suppose you mean Windows HANDLE data type? Do you to which kind of resource these handles refered?

In any case, I think adapting the tile size would be nicer than a hard limit, e.g. using 16 MB instead of 4 MB tiles above some specific number of tiles per page. I am not sure whether I will implement this for 0.4.14 depending on how difficult it is.

In any case, I'll merge this and make "TileItem::cancelRender" inline. I also think the signal-slot invocation using "QTimer::singleShot" within "TileItem::deleteAfterRender" could be replaced by a simple flag to further reduce overhead.

Thanks for the review! Best regards, Adam.

=== modified file 'sources/pageitem.cpp'
--- sources/pageitem.cpp	2015-02-01 13:27:13 +0000
+++ sources/pageitem.cpp	2015-02-01 13:33:19 +0000
@@ -334,10 +334,12 @@
     }
     else
     {
-        foreach(TileItem* tile, m_tileItems)
+        foreach(TileItem* tile, m_exposedTileItems)
         {
             tile->cancelRender();
         }
+
+        m_exposedTileItems.clear();
     }
 }
 
@@ -1121,6 +1123,8 @@
         m_tileItems.replace(index, new TileItem(this));
     }
 
+    m_exposedTileItems.clear();
+
     if(oldCount != newCount)
     {
         foreach(TileItem* tile, m_tileItems)
@@ -1173,15 +1177,25 @@
 
         foreach(TileItem* tile, m_tileItems)
         {
-            if(translatedExposedRect.intersects(tile->rect()))
+            const bool intersects = translatedExposedRect.intersects(tile->rect());
+            const bool contains = m_exposedTileItems.contains(tile);
+
+            if(intersects && !contains)
             {
-                tile->paint(painter, m_boundingRect.topLeft());
+                m_exposedTileItems.insert(tile);
             }
-            else
+            else if(!intersects && contains)
             {
+                m_exposedTileItems.remove(tile);
+
                 tile->cancelRender();
             }
         }
+
+        foreach(TileItem* tile, m_exposedTileItems)
+        {
+            tile->paint(painter, m_boundingRect.topLeft());
+        }
     }
 
     if(s_settings->pageItem().decoratePages() && !presentationMode())

=== modified file 'sources/pageitem.h'
--- sources/pageitem.h	2014-11-15 11:12:55 +0000
+++ sources/pageitem.h	2015-02-01 13:33:19 +0000
@@ -25,6 +25,7 @@
 #include <QCache>
 #include <QGraphicsObject>
 #include <QIcon>
+#include <QSet>
 
 class QGraphicsProxyWidget;
 
@@ -210,6 +211,7 @@
     void prepareGeometry();
 
     QVector< TileItem* > m_tileItems;
+    mutable QSet< TileItem* > m_exposedTileItems;
 
     void prepareTiling();