New tile unref logic and various leak fixes



Hello Pierre-Luc,

I've been hunting some memory leaks in libchamplain (well, the biggest
one actually seems to be a problem in my graphics driver that leaks
memory) and made a change that in my opinion significantly simplifies
memory handling of tiles. In principle, I did the following:

* made champlain_tile, champlain_zoom_level and champlain_polygon
inherit from ClutterGroup, which makes them ClutterActors - this means
that the actors that were inside them and that contained the actor
have disappeared - it's the object itself which is the actor (for
ChamplainTile there is still the contents actor inside, which is then
inserted into ChamplainTile). This simplifies a lot of things. Before
you had to have a list where you inserted tiles and in parallel, you
had to insert the actors within tiles into the parent actor. All you
do now is that you insert the tiles into parent actor since they are
actors themselves and use the functionality of ClutterCOntainer to
iterate over the tiles.

* I merged the functionality of champlain_map and champlain_zoom_level
into one class (champlain_zoom_level) - this simplifies addition and
removal of tiles inside. I have also eliminated the "double buffering"
thing from champlain_map - first it unnecessarily complicates
everything and second its only use case is when you zoom in and out
immediately, which is IMO rather infrequent use case to deserve this
optimisation. This also saves memory a bit since you don't have to
keep 2 tile layers in memory at the same time.

* tiles are not reffed/unreffed anywhere in the code. Instead, upon
their creation they are inserted into champlain_zoom_level and when
they are destructed, the map sources have to detect this destruction -
this is much simpler than other way round (you can look at the changes
inside map sources, they are mostly trivial). So the current
philosophy is "destroy tile whenever needed and let others detect
that". Before I was never sure if and when a tile will be destroyed
because of the complicated ref/unref relations. Now this problem goes
away - the tile will allays be destroyed inside
view_load_visible_tiles() in the part "Get rid of old tiles first" -
no matter in which state it is (loading or done).

* there are some random fixes here and there

The diffstat looks quite nice as well:

 champlain-gtk/gtk-champlain-embed.c       |    2 +-
 champlain/Makefile.am                     |    2 -
 champlain/champlain-base-marker.c         |    1 -
 champlain/champlain-error-tile-source.c   |    6 +-
 champlain/champlain-file-cache.c          |   25 +-
 champlain/champlain-layer.c               |    4 +-
 champlain/champlain-map.c                 |  126 ------
 champlain/champlain-map.h                 |   61 ---
 champlain/champlain-marker.c              |  152 +++++---
 champlain/champlain-marker.h              |    2 -
 champlain/champlain-memphis-tile-source.c |   88 +++--
 champlain/champlain-network-tile-source.c |   21 +-
 champlain/champlain-polygon.c             |  228 +++++++++---
 champlain/champlain-polygon.h             |   12 +-
 champlain/champlain-private.h             |   13 -
 champlain/champlain-tile.c                |  146 ++-----
 champlain/champlain-tile.h                |    7 +-
 champlain/champlain-view.c                |  614 ++++++++++-------------------
 champlain/champlain-view.h                |    3 -
 champlain/champlain-zoom-level.c          |  210 ++--------
 champlain/champlain-zoom-level.h          |   16 +-
 21 files changed, 669 insertions(+), 1070 deletions(-)

There are 400 lines less of code. Some explanations:

* champlain-map.* is gone
* the pluses in champlain-marker.c are mainly because previously
someone apparently forgot to unref/free memory in finalize and destroy
* the pluses in champlain-polygon.c are caused by moving
draw_polygon() from ChamplainView here
* champlain-tile.c, champlain-view.c and champlain-zoom-level.c just
got simpler because of the changes mentioned above.

Many of the diffs are mainly because I had to get rid of the "public
private" members to be able to understand the code a bit. Before you
were accessing the private members of classes through e.g.
polygon->priv->actor which was very hard for me to track, so I removed
the priv member from the class structure completely and used
GET_PRIVATE everywhere in the code instead. Unfortunately I did this
in the middle of coding (and and the very same applies for the rest of
the changes as well) so the result is one big patch - sorry for that.

These changes make it also possible to terminate tile loading while in
progress (I haven't implemented this yet but I definitely plan to do
that). I've always had a bad feeling that when you scroll fast enough
you may be unnecessarily downloading tiles that you visited on the
way, but which are not displayed in the view any more. Tile
downloading could be stopped now when the tile is destroyed (as I said
previously - it will be destroyed now even if the map source is still
loading data for it) and hooking to the "destroy" event of it and
calling soup_session_cancel_message() inside. Should be pretty
straightforward.

In ChamplainView there are two places where I wasn't sure what the
current implementation does - see lines 2759 and 3159 - I left some
comments there.

Now the question is:

1. Do you like it?
2. Would you accept it for 0.6 already?

The changes I made are completely compatible with existing
applications - none of the demos had to be modified. I must say I like
this approach much better than the previous one because the tile
creation/destruction is completely under our control now and
everything got just simpler. I'm aware that I'm coming with this
change quite late in the game, on the other hand the code seems to be
pretty stable and I'm not aware of any bugs. Please test the memory
usage - this is something that I cannot test now on my machine because
any texture loading leaks memory here (I don't have time now to
investigate how to fix the problem or reinstall linux).

The code is here:

http://gitorious.org/~techy/libchamplain/unref-clone

Prepare good arguments if you say you don't like this change ;-).

Jiri


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]