After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 779614 - Scrolling is very jerky/slow with Side Pane showing Outline
Scrolling is very jerky/slow with Side Pane showing Outline
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: PDF
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-05 14:43 UTC by ragnese
Modified: 2017-05-22 05:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ev-sidebar-links: Optimize reverse link lookup for a page (6.96 KB, patch)
2017-04-26 21:12 UTC, Benjamin Berg
none Details | Review
ev-sidebar-links: Optimize reverse link lookup for a page (6.90 KB, patch)
2017-04-29 12:18 UTC, Benjamin Berg
committed Details | Review

Description ragnese 2017-03-05 14:43:27 UTC
This is similar to a known bug that the Side Pane makes scrolling slow when showing thumbnails. It seems like Outline is also making scrolling slow.

It's not really slow- it's jerky. I'll scroll a little bit and it's fine, but it will suddenly stop completely for a second or two.
Comment 1 José Aliste 2017-03-06 12:47:29 UTC
Is this happening with every file? 
If not please attacha pdf sample
Comment 2 Germán Poo-Caamaño 2017-03-06 13:25:53 UTC
I have been bitten by this issue on and off. I do not remember if with the same document I can get the good and bad behaviour. Usually I was bitten by the issue when I was too busy to debug it, and when I was not busy I could not reproduce it :-/
Comment 3 Johannes Berg 2017-04-26 19:20:45 UTC
The following is in the source code:

/* We go through the tree linearly looking for the first page that
 * matches.  This is pretty inefficient.  We can do something neat with
 * a GtkTreeModelSort here to make it faster, if it turns out to be
 * slow.
 */

Yes. It does turn out to be slow - for example, grab 802.11-2012 from http://standards.ieee.org/about/get/802/802.11.html and scroll through it with the outline visible. It's much worse on the -2016 version, but sadly that's not available openly yet.

Note that the comment regarding GtkTreeModelSort is completely bogus - what's needed here isn't sorting, it's a reverse search going from page number to outline entry. A hashtable seems possible, but doesn't work because the outline will likely not have an entry for every page number, and fixing the problem of following properly in the outline while jumping to a page without an outline entry itself would be good.

A binary tree would likely be a good data structure for that, keeping a page number -> outline entry mapping for each first outline entry mapping on a page, and making the lookup go to the next smaller value if not found.
Comment 4 Johannes Berg 2017-04-26 19:51:17 UTC
g_tree_search() can be used by passing as user_ptr a struct with the target page number to find, and an *output* pointer to the outline entry. That pointer can be filled on every g_tree_search() invocation, and then be used if g_tree_search() returned NULL. If it returns non-NULL, prefer that value of course.
Comment 5 Benjamin Berg 2017-04-26 21:12:14 UTC
Created attachment 350507 [details] [review]
ev-sidebar-links: Optimize reverse link lookup for a page

For large documents the linear search for the first link that is on a
certain page is really slow. Because of this scrolling becomes slow
whenever the page changes.

Replace the linear search with a search in a binary tree populated with
the first link on each page and the corresponding GtkTreePath. This way
a specialized binary tree lookup can be used to find the closest
matching link and select that in the treeview.
Comment 6 Johannes Berg 2017-04-26 21:13:47 UTC
(In reply to Benjamin Berg from comment #5)
> Created attachment 350507 [details] [review] [review]
> ev-sidebar-links: Optimize reverse link lookup for a page

Tested on 3.22.1 - works great :)
Comment 7 Germán Poo-Caamaño 2017-04-27 01:36:41 UTC
Wow. Good job. It works good on master.
Comment 8 Carlos Garcia Campos 2017-04-29 09:57:34 UTC
Review of attachment 350507 [details] [review]:

Thanks! it looks good to me, I have only a few minor comments.

::: shell/ev-sidebar-links.c
@@ +504,2 @@
 	/* Widget is not currently visible */
+	if (!gtk_widget_is_visible (GTK_WIDGET (sidebar_links)))

Why this change? is_visible only means it's marked as visible, not necessarily that it's visible, that's why mapped is used here, I think.

@@ +590,3 @@
+			       gpointer      data)
+{
+	EvSidebarLinks *sidebar_links = (data);

I know this is existing code but the () are useless here.

@@ +598,3 @@
+			    -1);
+
+	if (link) {

I would use an early return here if !link

@@ +601,3 @@
+		int page;
+		GtkTreePath *existing;
+		EvDocumentLinks *document_links = EV_DOCUMENT_LINKS (sidebar_links->priv->document);

you can use priv here instead of sidebar_links->priv.

@@ +608,3 @@
+		/* Only save the first link we find per page. */
+		existing = g_tree_lookup (priv->page_link_tree, GINT_TO_POINTER (page));
+		if (!existing)

We don't need the local variable

if (!g_tree_lookup())
        g_tree_insert()
Comment 9 Benjamin Berg 2017-04-29 11:17:31 UTC
I agree with all the style fixes.

The gtk_widget_get_mapped still returns TRUE if the Overview is selected but the sidebar is hidden in evince. gtk_widget_is_visible does not seem to do it in this case. So it seems like a widget can still be mapped while being invisible

tbh. I am not sure about the semantics right now, but is_visible() should is safe and catches a case that was not caught before.
Comment 10 Benjamin Berg 2017-04-29 11:18:58 UTC
Oh, is_visible tests the whole widget hierarchy, not only the widget itself (which would be get_visible).
Comment 11 Benjamin Berg 2017-04-29 12:18:56 UTC
Created attachment 350727 [details] [review]
ev-sidebar-links: Optimize reverse link lookup for a page

For large documents the linear search for the first link that is on a
certain page is really slow. Because of this scrolling becomes slow
whenever the page changes.

Replace the linear search with a search in a binary tree populated with
the first link on each page and the corresponding GtkTreePath. This way
a specialized binary tree lookup can be used to find the closest
matching link and select that in the treeview.
Comment 12 Carlos Garcia Campos 2017-05-03 16:17:29 UTC
Review of attachment 350727 [details] [review]:

Excellent! I've just pushed it, thank you!
Comment 13 Johannes Berg 2017-05-05 08:56:33 UTC
The patch applies as-is to older versions, I've tested it on 3.22 for example - perhaps you'd be willing to also put it at least on 3.24 or perhaps also 3.22?

I know a number of people working on wifi who really have this problem with the 802.11-2016 spec (basically, evince isn't usable to view it at all), and having it deployed earlier than 3.26 (September?) would be really nice.

Thanks!
Comment 14 Carlos Garcia Campos 2017-05-21 07:57:41 UTC
(In reply to Johannes Berg from comment #13)
> The patch applies as-is to older versions, I've tested it on 3.22 for
> example - perhaps you'd be willing to also put it at least on 3.24 or
> perhaps also 3.22?
> 
> I know a number of people working on wifi who really have this problem with
> the 802.11-2016 spec (basically, evince isn't usable to view it at all), and
> having it deployed earlier than 3.26 (September?) would be really nice.
> 
> Thanks!

Sure, I've just backported it to 3.24 branch
Comment 15 Johannes Berg 2017-05-22 05:57:10 UTC
Thanks!