GNOME Bugzilla – Bug 710973
draw_tabs_and_spaces should not call gtk_text_view_get_iter_location excessively
Last modified: 2014-05-26 16:39:24 UTC
gtk_text_view_get_iter_location can be very slow, especially when wrapping is enabled (I filed bug 710972 about that). Calling it for every char can be a big slowdown. With draw_spaces enabled, gedit is basically unresponsive when scrolling through files with long lines and wrapping enabled. When disabling draw_spaces, it is still very slow, but at least useable.
Yes, it is a known issue. It would be better if pango can directly draw the spaces, if I understand correctly the problem.
Created attachment 258443 [details] [review] Stopgap solution So for every line, I bsearch the last visible char. Then iterate the line chars, checking whether the chars should be drawn at all (avoiding the get_iter_location call if not needed). Some rough manual testing gives me these timings: # base: nowrap: 0.37 wrap: 0.13 # old: nowrap: 0.72 wrap: 1.1 # patched: nowrap: 0.69 wrap: 0.3 So it is roughly 4x faster for the wrapped case, very small improvement for the non-wrapped case. Other changes are mostly noise.
Review of attachment 258443 [details] [review]: Here you have an initial review. ::: gtksourceview/gtksourceview.c @@ +2035,3 @@ +static gboolean +check_needs_drawing (GtkSourceView *view, rename it to just needs_drawing? @@ +2145,3 @@ + + *end_iter = *start_iter; + gtk_text_iter_forward_to_line_end(end_iter); space before ( @@ +2156,3 @@ + } + + min = gtk_text_iter_get_line_offset(start_iter); space before ( @@ +2157,3 @@ + + min = gtk_text_iter_get_line_offset(start_iter); + max = gtk_text_iter_get_line_offset(end_iter); space before ( @@ +2165,3 @@ + gtk_text_view_get_iter_location (text_view, end_iter, &rect); + + if (is_wrapping ? this is a bit hard to decrypt can you rewrite it with && and || ? @@ +2171,3 @@ + min = i + 1; + } + else if (is_wrapping ? same here @@ +2179,3 @@ + else + { + return; use break here? @@ +2262,3 @@ /* move to the first iter in the exposed area of * the next line */ + if (!gtk_text_iter_starts_line (&s)) if inside if, use && ?
Created attachment 258467 [details] [review] Review comments addressed Thanks a lot for the very fast review :-) And sorry I got the style wrong on the first try. btw: Are there any automated tests for this? It does work like a charm in manual testing, but I would feel a lot more confident if there were some tests backing this (and maybe even benchmarks?)
Created attachment 258791 [details] [review] Add profiling and debug output So I added some more instrumentation to these calls, here are some of the observations I made. Would be nice if someone could comment on the findings. I will continue profiling to figure out where the overhead time in the scrolling case is spent. # observations Within a frame, `gtk_source_view_paint_right_margin` is actually called 5 times. 4 of those have `gtk_cairo_should_draw_window (…) = FALSE`, so they don’t do any drawing at all. Together, they are below 1ms, so I didn’t include those timings here. The `paint time` measurement is what I observe when hooking a timer into the frame clocks `before_paint` and `after_paint` signals. Those times are considerably larger, so a lot of time is spend not in `gtk_source_view_draw` at all. When scrolling a wrapped view, without spaces, the redraw time is below 1ms, however there is a ~26ms overhead outside of `gtk_source_view_draw`. When scrolling with spaces enabled, the time spent in `draw_tabs_and_spaces` is roughly the same, meaning it does not support incremental redraw? # results ## non-wrapping, with spaces > gtk_source_view_draw start draw area: 0 - 962 lines to update: 0 - 9 Painting marks background for line numbers 0 - 9 gtk_source_view_paint_right_margin time: 0,014 (sec * 1000) > draw_tabs_and_spaces start drawing spaces for line 0, characters 0 - 339 drawing spaces for line 1, characters 0 - 339 drawing spaces for line 2, characters 0 - 339 drawing spaces for line 3, characters 0 - 339 drawing spaces for line 4, characters 0 - 339 drawing spaces for line 5, characters 0 - 339 drawing spaces for line 6, characters 0 - 339 drawing spaces for line 7, characters 0 - 339 drawing spaces for line 8, characters 0 - 339 drawing spaces for line 9, characters 0 - 339 draw_tabs_and_spaces time: 316,18 (sec * 1000) > draw_tabs_and_spaces end gtk_source_view_draw time: 453,095 (sec * 1000) > gtk_source_view_draw end paint time: 617,645 ## non-wrapping, without spaces > gtk_source_view_draw start draw area: 0 - 962 lines to update: 0 - 9 Painting marks background for line numbers 0 - 9 gtk_source_view_paint_right_margin time: 0,013 (sec * 1000) gtk_source_view_draw time: 137,064 (sec * 1000) > gtk_source_view_draw end paint time: 306,117 ## wrapping, with spaces > gtk_source_view_draw start draw area: 0 - 981 lines to update: 0 - 2 Painting marks background for line numbers 0 - 1 gtk_source_view_paint_right_margin time: 0,016 (sec * 1000) > draw_tabs_and_spaces start drawing spaces for line 0, characters 0 - 10000 drawing spaces for line 1, characters 0 - 7389 draw_tabs_and_spaces time: 185,877 (sec * 1000) > draw_tabs_and_spaces end gtk_source_view_draw time: 254,664 (sec * 1000) > gtk_source_view_draw end paint time: 307,443 ## wrapping, without spaces > gtk_source_view_draw start draw area: 0 - 981 lines to update: 0 - 2 Painting marks background for line numbers 0 - 1 gtk_source_view_paint_right_margin time: 0,014 (sec * 1000) gtk_source_view_draw time: 79,81 (sec * 1000) > gtk_source_view_draw end paint time: 132,113 ## scrolling wrapped, without spaces > gtk_source_view_draw start draw area: 98 - 1079 lines to update: 0 - 2 Painting marks background for line numbers 0 - 1 gtk_source_view_paint_right_margin time: 0,01 (sec * 1000) gtk_source_view_draw time: 0,664 (sec * 1000) > gtk_source_view_draw end paint time: 26,81 ## scrolling wrapped, with spaces > gtk_source_view_draw start draw area: 98 - 1079 lines to update: 0 - 2 Painting marks background for line numbers 0 - 1 gtk_source_view_paint_right_margin time: 0,014 (sec * 1000) > draw_tabs_and_spaces start drawing spaces for line 0, characters 1677 - 10000 drawing spaces for line 1, characters 0 - 9069 draw_tabs_and_spaces time: 199,304 (sec * 1000) > draw_tabs_and_spaces end gtk_source_view_draw time: 200,14 (sec * 1000) > gtk_source_view_draw end paint time: 227,478
(In reply to comment #5) > When scrolling a wrapped view, without spaces, the redraw time is below 1ms, > however there is a ~26ms overhead outside of `gtk_source_view_draw`. So the overhead comes from the gutter renderer: https://git.gnome.org/browse/gtksourceview/tree/gtksourceview/gtksourcegutterrenderertext.c#n106 Each one of those calls to gtk_text_view_get_iter_location costs ~13ms. The time spent in `gutter_renderer_text_draw` apparently amounts for all the difference between the time difference of `view_draw` and `paint time` that I mentioned earlier.
(In reply to comment #5) > When scrolling with spaces enabled, the time spent in `draw_tabs_and_spaces` > is roughly the same, meaning it does not support incremental redraw? So GtkTextView is using GtkPixelCache internally for incremental redraw. That class is private though and not exposed to outside gtk users :-( Any suggestions?
See the bug #708423 for a suggestion on how to draw on the pixelcache from GtkSourceView.
(In reply to comment #1) > It would be better if pango can directly draw the > spaces, if I understand correctly the problem. Ignacio, wasn't this the problem? Pango and GtkTextView don't have settings for drawing spaces. We can imagine a GtkTextTag property to enable the visibility of certain types of spaces in a certain region of the buffer. And pango would draw the spaces directly at the right position, instead of computing the position afterwards.
Yes, drawing spaces should be pushed down on the stack, in GtkTextView and Pango, see bug #721015 and bug #721014. In the meantime, a performance improvement would be nice though.
(In reply to comment #10) > Yes, drawing spaces should be pushed down on the stack, in GtkTextView and > Pango, see bug #721015 and bug #721014. > > In the meantime, a performance improvement would be nice though. Is there something blocking this from being merged? Would be nice to have some speedup before a *proper* solution comes along.
(In reply to comment #11) > Is there something blocking this from being merged? Would be nice to have some > speedup before a *proper* solution comes along. The time to review the patch :-) I'll try to have a look at it... if it falls again out of my todo list, keep poking me and sorry again for the delay
Review of attachment 258467 [details] [review]: Sorry again for the delay, the optimization makes sense to me, but I have not tested yet In the mean time here is a quick round of (mostly trivial) review. If you have time to update the patch it would be great ::: gtksourceview/gtksourceview.c @@ +2009,3 @@ { gunichar c; + GdkRectangle rect; leave a blank line after var declaration @@ +2035,3 @@ +static gboolean +needs_drawing (GtkSourceView *view, the function name is a bit too generic... maybe space_needs_drawing? @@ +2138,3 @@ + GtkTextIter *start_iter, + GtkTextIter *end_iter, + GdkPoint end, Just pass x and y as int @@ -2122,0 +2137,15 @@ +get_end_iter (GtkTextView *text_view, + GtkTextIter *start_iter, + GtkTextIter *end_iter, ... 12 more ... This is really hard to read... @@ -2160,2 +2226,3 @@ x2, y2); + GdkPoint endpoint = {x2, y2}; declare vars at the start of the block (but as said above you may as well simply not use GdkPoint)
Created attachment 268055 [details] [review] Updated I really have to learn those mad git skills, just wasted a lot of time trying to edit the first of the patches with `rebase -i` only to have it fail on me with tons of merge conflicts and then squashing both patches together even though i did not want that -_- So there you go with the profiling/debugging patch merged in.
I amended the patch a little bit (e.g removed the very specific debug instrumentation but kept the profile output) and pushed the patch to master. Thank you!
There is a regression, see bug #730713.