Created attachment 274046 [details] [review]
textiter: remove recursivity for find_by_log_attrs()

find_by_log_attrs() was a recursive function. It is replaced by an
iteration.

There is still a part to optimize for a later commit.

find_visible_by_log_attrs() should also be optimized, and is probably the main source of the bad performances. The function should skip directly all the invisible region.

Created attachment 274079 [details] [review]
textiter: small optimization for find_by_log_attrs()

Use gtk_text_iter_set_line_offset (&tmp_iter, 0) instead of
gtk_text_iter_get_line(). The difference should not be big. In the first
case the line doesn't need to be traversed thanks to the offset 0. For
get_line(), the btree must be traversed.

A temporary iter is needed to not break the behavior. But the behavior
is quite strange, the function works directly on the iter passed as an
argument to the function, even if the function returns FALSE (not
found). So maybe a later commit will fix this strange behavior.

Created attachment 274088 [details] [review]
textiter: simplify find_by_log_attrs()

And find_visible_by_log_attrs().

The already_moved_initially parameter was always FALSE.

There is also a small cleanup of the find_visible_by_log_attrs()
(remove trailing spaces, fix indentation).

It seems there is no easy way to skip an invisible region.

A solution:
- create a list of GtkTextTags having the 'invisible' property to TRUE (and 'invisible-set' too), by looking at all the tags present in the tag table.
- call gtk_text_iter_forward_to_tag_toggle() or backward_to_tag_toggle() on each tags.
- take the nearest iter.

(In reply to comment #5)
> - take the nearest iter.

This won't work if the invisible tags overlap, unfortunately.

A GtkTextRegion would be useful to solve this problem I think:
https://git.gnome.org/browse/gtksourceview/tree/gtksourceview/gtktextregion.h

We add all the nearest invisible regions in the GtkTextRegion, GtkTextRegion will merge the regions if they overlap, and then we use the iterator functions to find the nearest visible region.

Do you have any performance tests for this ? It would be really nice to have tests that prove the claimed performance improvements.

Created attachment 274262 [details]
Test performances

The first patch is not only for performances reasons, an iteration is better in C than recursivity.

The second patch is a micro-optimization which was based on common sense. The attached file test.c measures this micro-optimization. The results:
$ ./test
Before patch: 0.196737 seconds.
After patch: 0.192029 seconds.

It tests the difference between:
> gtk_text_iter_set_line_offset (&iter, 0);
and:
> gtk_text_iter_get_line (&iter) > 0

So I was right.

And the third patch is just a cleanup.

But the main problem still exists in find_visible_by_log_attrs() (based on common sense).

So the fixme comment "this function is very, very gratuitously slow" was probably no longer valid. Or am I missing something?

The slower thing in find_by_log_attrs() is the call to find_line_log_attrs(), which call _gtk_text_buffer_get_line_log_attrs(). Getting the log attrs for the whole line is needed because some attributes need some context to work properly.

The other things in find_by_log_attrs() is just navigation through the buffer. It can not be really optimized (and doing so is useless, as the micro-optimization above shows).

So I think it is right to remove the fixme comment about the slowness of find_by_log_attrs().

Review of attachment 274079 [details] [review]:

This one looks obviously correct to me. And I agree that the usage of the iter is suspicious

Review of attachment 274046 [details] [review]:

I am not sure that the comment about being slow is outdated or if instead there is some logical optimization we are missing (as opposed to playing games with recursivity and microoptimizations)
It is also true that keeping that comment without anyone knowing what it means is not very useful eiter

That said, I think that the non recursive version is actually easier to read, so I think the patch should go in

::: gtk/gtktextiter.c
@@ +3113,3 @@
+      gboolean found;
+
+      found = find_line_log_attrs (iter, func, &offset, already_moved_initially);

if you want to shorten the code a bit here you could do

if (!find_line_log_attrs())
   ...
   return

and save a local var

Review of attachment 274088 [details] [review]:

This is strange... the patch itself looks ok, but it would be interesting to dig into the git log when that param started to be false for every call

Review of attachment 274088 [details] [review]:

Actually, the bool param was used in the recursion. So I think it is more clear to squash this patch with the first one

Comment on attachment 274088 [details] [review]
textiter: simplify find_by_log_attrs()

This commit has been squashed with the commit that removes the recursivity.

(In reply to comment #5)
> It seems there is no easy way to skip an invisible region.

Bug #321548 contains an interesting solution, that can probably be applied here.

Though the solution described in Comment #5 and Comment #6 could be generalized so it is useful for other properties, which would be useful for bug #719978.

We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.

As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new