On Wed, 2004-09-15 at 16:37, bulia byak wrote:

On Wed, 15 Sep 2004 16:15:51 -0300, bulia byak <buliabyak@...400...> wrote:

...

I have a rather big and complex document (500 Kb SVG) containing many icons which I export individually. I noticed that after each export the inkscape task grabs about 5% of my total memory (512 Mb) and never releases it.

Here's another way to reproduce that: run Inkscape with another document and from it, open and close that 500 Kb document. This eats even more memory, about 7%, though after some time 1% is freed back and only 6% remain grabbed permanently. Every open/close of the document consistenly adds 6% to the total memory occupied by Inkscape. So it looks like something somewhere is not freed when it should be freed.

While it does sound like there may be a problem with memory leakage, as we debug this, please bear in mind that even under normal circumstances there will be some slush, because:

* malloc() claims memory from the OS in large blocks and divvies them up to satisfy smaller allocations; those blocks cannot be returned to the OS unless every single allocated region within them is freed

(even then, it cannot always be returned to the OS due to other contraints, such as those brk() is subject to on Unix systems)

* the garbage collector inherently introduces a small amount of slush, since it doesn't free memory immediately after it becomes unused

Such memory will eventually get reused, however, so in the absence of leaks you are not likely to see the kind of extreme growth you are seeing.

IIRC, valgrind can identify the source of memory leaks, so that might be worth a go.

-mental

Mental, another probably boehm-related trouble is the stream of warnings on OSX reported by Fred:

[12:38:52] <yafojab> hi [12:40:33] <yafojab> is getting a zillion "deallocation of a pointer not malloced" normal? is there something to do to get rid of them? [12:50:13] <bbyak> where do you see these warnings? [12:50:44] <yafojab> when starting inkscape [12:51:06] <yafojab> it takes 2 minutes to load because of these.

On Fri, 17 Sep 2004 15:24:06 -0300, bulia byak <buliabyak@...400...> wrote:

I think I found the root of the problem. I compiled 0.39 and tested, and it does not exhibit this kind of behavior. Memory consumption grows by 0.4% on first export and stays the same on subsequent exports.

In 0.39, we did have livarot the same as now, but we didn't yet use boehm. So I think it's highly likely that the boehm gc does not do its work properly. Maybe it collects the garbage, but it does not seem to free it.

...

• the garbage collector inherently introduces a small amount of slush, since it doesn't free memory immediately after it becomes unused

And no, it does not free it afterwards. I let Inkscape grab 60% memory and left it on overnight. In the morning, it still held the same 60%.

So, Mental, could you please look into it? Perhaps we're misconfiguring libgc or not using it properly. It's a VERY annoying problem which needs urgent resolution. For example it makes previews in the open dialog unusable: each preview loads the document, and since the memory is not freed, it quickly eats up all available memory as you go through the files in a directory. Just go through a couple dozen files, and you're lucky if you manage to kill Inkscape before it bogs the system down to a complete halt.

Based on past experience, I wouldn't be so quick to blame boehm.

I'm not necessarily blaming boehm for this particular one. It's just some memory-related problem.

However the memory eating problem is, I think, obviously traceable to boehm. It may collect the garbage but it does not seem to recycle it, quickly turning all of my memory into a landfill :)

On Sun, 2004-09-19 at 01:05, MenTaLguY wrote:

...

For example it makes previews in the open dialog unusable: each preview loads the document, and since the memory is not freed, it quickly eats up all available memory as you go through the files in a directory. Just go through a couple dozen files, and you're lucky if you manage to kill Inkscape before it bogs the system down to a complete halt.

This sounds like a good steps-to-reproduce. Thanks.

Ok, I have at least partly tracked this down.

It looks like the SPDocuments created for the previews never get freed. I set breakpoints in gdb, caught one document as it was created, and saved a pointer to it in a temporary location. Then I clicked on several other files and got previews.

The older preview document remained live, and its GObject::ref_count never dropped below 1.

Apparently setFileName() creates a document, but neglects to release its ref on that document when it is finished.

SPDocuments, like all refcounted objects in Inkscape, are created with an initial refcount of 1. In general, this means you ought to do this sort of thing:

foo = create_foo(); // create a foo ... // do what we need to do with foo bar->withFoo(foo); // can take its own reference if it needs to ... // do anything else we need to do with foo unref_foo(foo); // we should let go of our reference now

It's the equivalent of the last line that was missing here.

There are probably other similar leaks throughout the codebase. I remember having to fix several such leaks in repr-related code a while back.

SPDocuments just happen to be the worst possible (and most noticable) case, since a single document references a LOT of memory.

I've comitted a fix for this to CVS; see if things get any better for you. I expect there are more layers to this particular problem.

-mental

On Sun, 2004-09-19 at 02:54, bulia byak wrote:

...

foo = create_foo(); // create a foo ... // do what we need to do with foo bar->withFoo(foo); // can take its own reference if it needs to ... // do anything else we need to do with foo unref_foo(foo); // we should let go of our reference now

It's the equivalent of the last line that was missing here.

I may misunderstand something, but, if we still need that last line, how it's better than not having a garbage collector at all? Before you needed an explicit free(), now you need an explicit unref() in the same place. If it's still not completely automatic, then what's the gain?

unref() is very diffrent from free() -- it says "destroy this object if nobody else is using it", not just "destroy this object". free() would never have been appropriate there.

That's the way reference counting has always worked. SPDocuments aren't yet managed by the garbage collector.

We will still need reference counting in places where gc-managed memory is referenced by non-gc-managed memory (this is the purpose of Inkscape::GC::Anchored), but only along that boundary. "Within the family", refcounts are no longer needed.

SPRepr is a good example of this: SPObjects, which are not gc-managed, refcount them, but SPReprs no longer need to refcount each other. That has resulted in significant simplification of the SPRepr code, which is the desired advantage.

As more classes come to be managed by the collector, less use of refcounts will be necessary, and we can even use I::G::A less[1].

It still eats memory pretty fast as you go through previews. Maybe slower than before (I haven't measured that before) but not by much. On export, there's no improvement at all (I did measure that before and after).

I think the next problem is that I screwed up and GC_invoke_finalizers() isn't being called in the idle loop like I had intended[2], so Inkscape::GC::Finalized objects never get collected.

One of the few I::G::F-derived classes is SPReprDoc, so obviously that is pretty bad.

It may be simpler to set GC_finalize_on_demand to 0 in Inkscape::GC::init (so finalizers get called immediately), and not bother with GC_invoke_finalizers().

-mental

[1] I::G::A does start with an initial reference count of 1 that you need to "release" (unref), but the requirements are much less stringent than with refcounts -- e.g. this is safe to do (and probably preferred):

using Inkscape::GC::release;

Foo *foo = release(new Foo()); foo->whatever(); ... etc

release() is a little weaker than unref() -- it just means "it's safe for the garbage collector to make a decision about this object". Here, there is no risk of the object being immediately destroyed, since the garbage collector knows you still have a pointer.

Just be sure to Inkscape::GC::anchor() the object if you stick a pointer to it in a place that isn't either a local variable or allocated using the gc-managed allocator.

The only reason I initially anchor I::G::As is that I need it to provide a drop-in replacement for the existing refcounting schemes that all start with an initial refcount of 1 -- e.g. sp_repr_ref() and sp_repr_unref() are now just wrappers for anchor() and release().

[2] I wanted to defer finalizer execution until the idle loop just to be extra conservative. It probably won't hurt if finalizers are called at other times.

On Sun, 2004-09-19 at 13:39, MenTaLguY wrote:

...

It may be simpler to set GC_finalize_on_demand to 0 in Inkscape::GC::init (so finalizers get called immediately), and not bother with GC_invoke_finalizers().

I just checked in this change to CVS.

This does not help unfortunately.

BTW, I realized the uncollected memory sizes that I reported to you were about 2Mb per each export. Which is about 0.5% of the total memory. So even if this gets collected, it won't affect much the 5% grab that happens on each export.

(replying to myself)

On Tue, 2004-09-21 at 00:58, MenTaLguY wrote:

On Sun, 2004-09-19 at 21:24, bulia byak wrote:

...

...

I just checked in this change to CVS.

This does not help unfortunately.

Interesting. It should have some. I wonder whether finalizers are getting called at all... :/

It seems they are not. In fact, Inkscape::GC::Finalized objects are simply not getting collected.

It looks like I made another really dumb mistake -- an I::G::F object, when created, needs to register a callback function to handle its cleanup before it is collected.

The problem? It saves a pointer to itself as a parameter, so the collector sees there is still a pointer to it and never collects it or any other objects it points to.

The correct approach is to save an integer offset relative to a base address instead, as is done by the (roughly equivalent) gc_cleanup class that ships with libgc.

So ... most or all of the leakage is coming from the libc heap, not boehm's. I guess that exonorates the collector itself, though the shim code and other changes I made to the tree to accomodate it are still suspect.

I::G::F = shim code

Some I::G::F-derived classes allocate chunks of memory from the traditional heap too -- notably the NRArena stuff, which is used so heavily by export -- so this could easily be bloating the malloc() heap.

I'll commit this and we'll see if it fixes everything.

-mental

On Tue, 2004-09-21 at 19:22, bulia byak wrote:

...

The problem? It saves a pointer to itself as a parameter, so the collector sees there is still a pointer to it and never collects it or any other objects it points to.

Wouldn't it be more logical if GC simply disregarded any pointers from an object to _itself_ and not count them as pointers?

It does.

The finalization callback "user data" pointers are saved elsewhere.

Actually, I'm oversimplifying a lot -- a mark-and-sweep collector doesn't care who points to what or how many they are, just whether an object is directly or indirectly accessible from a set of "root" memory locations by following pointers.

The callback data pointers get stored in a place that's accessible from one of the roots. This makes sense normally, since you don't want the callback's data to get collected prematurely.

Calling C++ destructors is slightly outside the normal usage of the finalization facility.

It's documented that you aren't supposed to pass a pointer to the finalizable block itself there directly. I missed that the first time I read the documentation.

...

The correct approach is to save an integer offset relative to a base address instead, as is done by the (roughly equivalent) gc_cleanup class that ships with libgc.

This doesn't strike me as something particularly elegant. Pointers to integers and back sounds like a crime in C, for obvious reasons. Even if you don't try to do arithmetic on them.

Storing a pointer as an integer wouldn't hide it from the collector anyway.

What I'm doing now is storing an integer offset (in bytes) from the start of the allocated block (which the collector tells us at finalization time), instead of a direct pointer.

This is just so we can find the C++ vtable entry for the destructor; a lot of the "magic" here is really imposed upon us by C++.

-mental