GNOME Bugzilla – Bug 554171
add fast path for list models in gailtreeview.c:get_row_count
Last modified: 2011-06-23 02:32:09 UTC
On startup, rhythmbox adds thousands of rows to various tree views. When accessibility is enabled, it often (not sure why it's not always) spends tens of minutes of aggregate CPU time in get_row_count() as a result of the gail_tree_view_get_n_children() call in gail_tree_view_ref_child(). More than half of it is actually in the GObject/interface overhead (type checking, casting) involved in two method calls per row, but that's neither here nor there.. Adding a fast path for list-only models (which all of rhythmbox's models are), like the one in gail_tree_view_get_n_rows(), reduces this overhead dramatically and makes the whole thing only noticeably slower than with accessibility off, rather than alarmingly slower.
Created attachment 119527 [details] [review] obvious patch
The right fix here would be for gail to not constantly ask about the number of rows (and columns) and do per-row things, obviously...
E.g. if you look at gailtreeview.c:columns_changed, it calls get_row_count inside a loop (!). In general, it seems that gailtreeview should be able to maintain the counts of rows and columns internally, without ever iterating over the whole model to find out.
I thought about doing that, but hit on this solution as I was figuring out where I'd need to update the row count. Guess I'll take another look..
Created attachment 119860 [details] [review] maintain row counts I haven't looked at column counts yet. I've run this for a little while, having it compare results with the existing code and report any differences, and it seems to work.
Created attachment 119889 [details] [review] slightly better This version sets the row count to 0 when there is no model for the tree view.
Finally getting around to looking at the column counts now, it looks like storing the actual column count (for get_n_actual_columns) is easy enough, but the visible column count is tricky. The tree view itself doesn't emit a signal when a column's visibility changes, so the gail tree view would need a signal handler for notify::visible for each column, which seems a bit complicated.
Created attachment 163715 [details] [review] patches
Needs testing but fixed a number of problems showing up in sysprof of the gdm greeter with a large number of users. Remaining problem spots include traverse_cells() and bonobo gunk.
Committed after testing with our new accessibility-dump and tree-performance tests. Thanks !