Re: [Gimp-developer] Big Fat Piggy Gimp
On Tue, 12 Jun 2001, Sven Neumann wrote: Hi, Big, Fat Piggy Gimp Fans, here's a very small patch that should fix our huge leak: [...] I don't consider this a clean solution but since it's a very small change, we should be able to evaluate easily if it is a correct fix. A better fix can be found in CVS HEAD, but I would prefer not to do too much changes to 1.2 if it can be avoided. I haven't tested the patch much so far, so please torture it. Hi Sven, would it be possible to provide a 1.2.2-alpha tarball by the end of the week so that people who want to test it can download the sources even if they do not have access to CVS? I would like to test the latest code (including the other patches) under Solaris, but I cannot access CVS from work (firewall problems). I can use CVS from home (and I do, but only for the HEAD branch) but it is much slower there. I suppose that I am not the only one in this case, so doing an alpha release could be helpful. Maybe not on ftp.gimp.org if we do not want it to be public yet, but on some other site announced on this list. -Raphael ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Big Fat Piggy Gimp
Layer deletes are a little funky: they don't get deleted when they are removed from the layer list, since they get pushed on the undo stack. They get deleted when the undo information is freed, ie when it expires or when it is on the redo stack and an new action is pushed on the undo stack, thus blowing the redo stack. Messing with reference counts might be an attempt to achieve this; I don't know. It's worth checking, if only to discount it. I also think such messing is really ugly, and as we're discovering, totally unmaintainable. Fixing a leak of this magnitude is definitely cause for another stable release, I think. It goes without saying that it should be widely and thoroughly tested, possibly by a 1.2.2alpha pre-release snapshot or something. Austin ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Big Fat Piggy Gimp
Hi, Garry R. Osgood [EMAIL PROTECTED] writes: What bemuses me is this sink-but-not-really-sink policy for layers and channels that prevails around 1.2.1 tile mismanagement, and which finds partial realization in layer_ref(). [layer.c-CVS-1.72.2.1 gimp-1-2] This brief little snippet explicitly increments the object reference count of layers (gtk_object_ref()) before invoking gtk_object_sink(). After this intrusion into private GTK object management has been pulled off, we are left with a special layer that is not floating, but not quite sunk either (it has a positive reference count). calling gtk_object_ref(); gtk_object_sink(); is definitely correct and not strange at all. It is what containers that take ownership of an object are supposed to do. I'm not saying that everything is fine (it apparantly is not), but I wouldn't say we are messing with the GTK+ object system here. The strange manipulations of layer_ref() is limited to gimp_image_add_layer(), where it is applied on layers that are being addedto the list in GimpImage::layers Yep. GimpImage::layers is a container and thus sinks the floating object after referencing it to make clear that this object hads found its home and now belongs to the container. Any attempt to delete it using layer_delete() will fail now since the layer is associated to an image and bad things will happen if you delete it without removing it from the container. This is what the comment in layer.c (around line 590) tries to make clear. The problem is most probably located in the undo code. If you remove a layer from an image the image should drop its reference to it. To avoid that it gets deleted and thus can never be readded by an Undo operation the undo stack should reference the layer before removing it from the image. The undo system then has to take care of dropping its reference as soon as the relevant undo step is removed from the stack. I guess this is where the real problem is located. Happy Gimp. The motivation for this layer_ref() trickery appears to date to Feb - August 1998 (Gimp 0.99.x) when Scott Goehring first Objectified Gimp drawables. GTK was in flux then as well, and I surmise (reading mail from the period is not clear on this) that GTK object referencing was a bit queer anyway. If so, is layer_ref() (and its effects) a kludge that has outlived its usefulness? Quite frankly, I get better memory management when I disable this non-standard reference manipulation and leave GTK object management internals undisturbed. You will get very bad effects if someone calls layer_delete() on a layer that has been added to an image and on the other hand you want to allow to delete layers that are not part of an image. This situation is exactly why the concept of floating objects was introduced to GTK+ and I think we are using it the way it was designed for. Personally, I think this is a must-fix that justifies a 1.2.2 release, Yes, we should definitely fix this. I'll try to have a closer look later today. Salut, Sven ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Big Fat Piggy Gimp
Hi, Big, Fat Piggy Gimp Fans,
here's a very small patch that should fix our huge leak:
Index: app/gimpimage.c
===
RCS file: /cvs/gnome/gimp/app/Attic/gimpimage.c,v
retrieving revision 1.121.2.1
diff -u -r1.121.2.1 gimpimage.c
--- app/gimpimage.c 2000/12/27 17:55:19 1.121.2.1
+++ app/gimpimage.c 2001/06/12 00:00:57
@@ -1456,7 +1456,7 @@
for (list = gimage-layers; list; list = g_slist_next (list))
{
layer = (Layer *) list-data;
- layer_delete (layer);
+ layer_unref (layer);
}
g_slist_free (gimage-layers);
g_slist_free (gimage-layer_stack);
@@ -1472,7 +1472,7 @@
for (list = gimage-channels; list; list = g_slist_next (list))
{
channel = (Channel *) list-data;
- channel_delete (channel);
+ channel_unref (channel);
}
g_slist_free (gimage-channels);
}
Given the confusing API used here it's not surprising that this could happen.
Gimp HEAD already had this hole plugged, but not because someone found and
fixed it, but because the code has already become much cleaner and we avoid
to wrap around basic functions like gtk_object_[ref|unref].
I don't consider this a clean solution but since it's a very small change,
we should be able to evaluate easily if it is a correct fix. A better fix
can be found in CVS HEAD, but I would prefer not to do too much changes to
1.2 if it can be avoided.
I haven't tested the patch much so far, so please torture it.
Salut, Sven
___
Gimp-developer mailing list
[EMAIL PROTECTED]
http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
