You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2004/11/02 21:53:27 UTC
APR hash table memory use [was: r11556 causes enormous fsfs memory
use]
Peter N. Lundblad wrote:
> Let's see if we a) mean the same by insert, remove, ... and b) read the
> code the same way:-)
>
> 1. hash_set(..., non-NULL): entry not found, allocate.
> 2. hash_set(..., NULL) (remove): entry found, remove from linked list
> 3. hash_set(..., non-NULL): entry not found, allocate
> ...
>
> If you do this, the hash pool will grow for each set. (The above is for
> the same key). We can discuss whose leak it was, but it isn't anymore,
> since I eliminated that usage.
Thanks for clarifying. I understand now:
Every time apr_hash_set(..., non-NULL) adds a new entry, it allocates some pool
memory for that entry and links it into one of its linked lists. When
apr_hash_set(..., NULL) deletes an entry, it unlinks it but does not give that
memory back to the pool.
So this is an inefficiency in APR's implementation of hash tables. It is not
exactly a "leak", since it is contained with a pool, but it is unbounded memory
use caused by repeated simple actions - not a good property for a container to
have.
It would be possible to fix this in APR if people thought it worthwhile, e.g.
by using a "free" list. (Maintain, in the hash table, a list of free entries,
initially empty. When deleting an entry, move it to the "free" list. When a
new entry is wanted, take one from the "free" list if available, else allocate
it from the pool.) This would make the memory used by a hash table
proportional to the maximum number of entries that it had ever held
simultaneously, rather than the number of insertions that had ever been performed.
It seems to me that the APR developers ought to be interested in doing this.
Will I make the effort to take this to their mailing list? Don't know.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] APR hash table memory use
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
> Thom May wrote:
>
>> Committed to HEAD, thanks.
>> -Thom
>>
>> * Julian Foad (julianfoad@btopenworld.com) wrote :
>>
>>
>>> The Subversion project encountered a usage pattern of APR hash
>>> tables that caused many megabytes of memory to be occupied by a
>>> small hash table after a sequence of many thousands of (insert,
>>> delete) pairs. They have subsequently changed the usage pattern to
>>> avoid this.
>>>
>>
> And I wanted to merge this to the 0.9 branch, now it turns out I don't
> know my password because I've always used ssh access...
Doh! Figured it out. Merged to the 0.9 branch in r65594.
Sorry for the noise.
(Although I do think that a small post to the list about the conversion
and what to do about access wouldn't have hurt; not all of us are at the
hackathon...)
-- Brane
Re: [PATCH] APR hash table memory use
Posted by Branko Čibej <br...@xbc.nu>.
Thom May wrote:
>Committed to HEAD, thanks.
>-Thom
>
>* Julian Foad (julianfoad@btopenworld.com) wrote :
>
>
>>The Subversion project encountered a usage pattern of APR hash tables that
>>caused many megabytes of memory to be occupied by a small hash table after
>>a sequence of many thousands of (insert, delete) pairs. They have
>>subsequently changed the usage pattern to avoid this.
>>
>>
And I wanted to merge this to the 0.9 branch, now it turns out I don't
know my password because I've always used ssh access...
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] APR hash table memory use
Posted by Branko Čibej <br...@xbc.nu>.
Thom May wrote:
>Committed to HEAD, thanks.
>-Thom
>
>* Julian Foad (julianfoad@btopenworld.com) wrote :
>
>
>>The Subversion project encountered a usage pattern of APR hash tables that
>>caused many megabytes of memory to be occupied by a small hash table after
>>a sequence of many thousands of (insert, delete) pairs. They have
>>subsequently changed the usage pattern to avoid this.
>>
>>
And I wanted to merge this to the 0.9 branch, now it turns out I don't
know my password because I've always used ssh access...
-- Brane
Re: [PATCH] APR hash table memory use
Posted by Thom May <th...@planetarytramp.net>.
Committed to HEAD, thanks.
-Thom
* Julian Foad (julianfoad@btopenworld.com) wrote :
> The Subversion project encountered a usage pattern of APR hash tables that
> caused many megabytes of memory to be occupied by a small hash table after
> a sequence of many thousands of (insert, delete) pairs. They have
> subsequently changed the usage pattern to avoid this.
Re: [PATCH] APR hash table memory use
Posted by Sander Striker <st...@apache.org>.
On Tue, 2004-11-02 at 17:22, Julian Foad wrote:
> Prevent unbounded memory use during repeated operations on a hash table.
>
> The hash table was allocating new memory for each new insertion of an entry,
> and not reclaiming it on deletions, so repetition of (insert, delete) caused
> the memory use to keep growing. This fix causes the memory freed by a deletion
> to be reused by a subsequent insertion, so that the memory used by the hash
> table is proportional to the maximum number of entries that it has ever held
> simultaneously, rather than the number of insertions that have been performed.
+1.
Sander
Re: [PATCH] APR hash table memory use
Posted by Julian Foad <ju...@btopenworld.com>.
> Julian Foad wrote:
>> Prevent unbounded memory use during repeated operations on a hash table.
Sander Striker wrote:
> +1.
Branko Čibej wrote:
> This looks good. I vote we apply it on both HEAD and the 0.9.x branch.
Can I do anything further to get this applied?
- Julian
Re: [PATCH] APR hash table memory use
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> Julian Foad wrote:
>> Prevent unbounded memory use during repeated operations on a hash table.
[...]
> This looks good. I vote we apply it on both HEAD and the 0.9.x branch.
Thanks. I have now checked that it passes Subversion's "make check", and, in a
debugger, that the new code is actually executed.
I would be very happy if someone would do an overall memory usage check with a
very large operation (e.g. in Subversion without the patch that avoids this usage).
- Julian
Re: [PATCH] APR hash table memory use
Posted by Julian Foad <ju...@btopenworld.com>.
> Julian Foad wrote:
>> Prevent unbounded memory use during repeated operations on a hash table.
Sander Striker wrote:
> +1.
Branko Čibej wrote:
> This looks good. I vote we apply it on both HEAD and the 0.9.x branch.
Can I do anything further to get this applied?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] APR hash table memory use
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> Julian Foad wrote:
>> Prevent unbounded memory use during repeated operations on a hash table.
[...]
> This looks good. I vote we apply it on both HEAD and the 0.9.x branch.
Thanks. I have now checked that it passes Subversion's "make check", and, in a
debugger, that the new code is actually executed.
I would be very happy if someone would do an overall memory usage check with a
very large operation (e.g. in Subversion without the patch that avoids this usage).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] APR hash table memory use
Posted by Branko Čibej <br...@xbc.nu>.
Julian Foad wrote:
>Prevent unbounded memory use during repeated operations on a hash table.
>
>The hash table was allocating new memory for each new insertion of an entry,
>and not reclaiming it on deletions, so repetition of (insert, delete) caused
>the memory use to keep growing. This fix causes the memory freed by a deletion
>to be reused by a subsequent insertion, so that the memory used by the hash
>table is proportional to the maximum number of entries that it has ever held
>simultaneously, rather than the number of insertions that have been performed.
>
>* apr/tables/apr_hash.c:
> (apr_hash_t): Add new field "free", a free-list.
> (apr_hash_make, apr_hash_copy, apr_hash_merge): Initialise the free-list.
> (find_entry): Use an entry from the free-list if there is one.
> (apr_hash_set): Return a deleted entry to the free-list.
>
>
This looks good. I vote we apply it on both HEAD and the 0.9.x branch.
-- Brane
Re: [PATCH] APR hash table memory use
Posted by Thom May <th...@planetarytramp.net>.
Committed to HEAD, thanks.
-Thom
* Julian Foad (julianfoad@btopenworld.com) wrote :
> The Subversion project encountered a usage pattern of APR hash tables that
> caused many megabytes of memory to be occupied by a small hash table after
> a sequence of many thousands of (insert, delete) pairs. They have
> subsequently changed the usage pattern to avoid this.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] APR hash table memory use
Posted by Sander Striker <st...@apache.org>.
On Tue, 2004-11-02 at 17:22, Julian Foad wrote:
> Prevent unbounded memory use during repeated operations on a hash table.
>
> The hash table was allocating new memory for each new insertion of an entry,
> and not reclaiming it on deletions, so repetition of (insert, delete) caused
> the memory use to keep growing. This fix causes the memory freed by a deletion
> to be reused by a subsequent insertion, so that the memory used by the hash
> table is proportional to the maximum number of entries that it has ever held
> simultaneously, rather than the number of insertions that have been performed.
+1.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] APR hash table memory use
Posted by Branko Čibej <br...@xbc.nu>.
Julian Foad wrote:
>Prevent unbounded memory use during repeated operations on a hash table.
>
>The hash table was allocating new memory for each new insertion of an entry,
>and not reclaiming it on deletions, so repetition of (insert, delete) caused
>the memory use to keep growing. This fix causes the memory freed by a deletion
>to be reused by a subsequent insertion, so that the memory used by the hash
>table is proportional to the maximum number of entries that it has ever held
>simultaneously, rather than the number of insertions that have been performed.
>
>* apr/tables/apr_hash.c:
> (apr_hash_t): Add new field "free", a free-list.
> (apr_hash_make, apr_hash_copy, apr_hash_merge): Initialise the free-list.
> (find_entry): Use an entry from the free-list if there is one.
> (apr_hash_set): Return a deleted entry to the free-list.
>
>
This looks good. I vote we apply it on both HEAD and the 0.9.x branch.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] APR hash table memory use
Posted by Julian Foad <ju...@btopenworld.com>.
The Subversion project encountered a usage pattern of APR hash tables that
caused many megabytes of memory to be occupied by a small hash table after a
sequence of many thousands of (insert, delete) pairs. They have subsequently
changed the usage pattern to avoid this.
The cause of the problem is that every time apr_hash_set(..., non-NULL) adds a
new entry, it allocates some new pool memory for that entry and links it into
one of its linked lists. When apr_hash_set(..., NULL) deletes an entry, it
unlinks it but never re-uses that memory.
So this is an inefficiency in APR's implementation of hash tables. It is not
exactly a "leak", since it is contained with a pool, but it is unbounded memory
use caused by repeated simple actions - not a good property for a container to
have.
It would be possible to fix this in APR if people thought it worthwhile, e.g.
by using a "free-list". (Maintain, in the hash table, a list of free entries,
initially empty. When deleting an entry, move it to the free-list. When a new
entry is wanted, take one from the free-list if available, else allocate it
from the pool.) This would make the memory used by a hash table proportional
to the maximum number of entries that it had ever held simultaneously, rather
than the number of insertions that had ever been performed.
In fact, using a free-list seems to be quite easy, with virtually no speed
penalty. A patch is attached, prepared in the manner of a Subversion patch.
(Please copy replies to dev@subversion.tigris.org, as I am not subscribed to
dev@apr.apache.org)
- Julian
[PATCH] APR hash table memory use
Posted by Julian Foad <ju...@btopenworld.com>.
The Subversion project encountered a usage pattern of APR hash tables that
caused many megabytes of memory to be occupied by a small hash table after a
sequence of many thousands of (insert, delete) pairs. They have subsequently
changed the usage pattern to avoid this.
The cause of the problem is that every time apr_hash_set(..., non-NULL) adds a
new entry, it allocates some new pool memory for that entry and links it into
one of its linked lists. When apr_hash_set(..., NULL) deletes an entry, it
unlinks it but never re-uses that memory.
So this is an inefficiency in APR's implementation of hash tables. It is not
exactly a "leak", since it is contained with a pool, but it is unbounded memory
use caused by repeated simple actions - not a good property for a container to
have.
It would be possible to fix this in APR if people thought it worthwhile, e.g.
by using a "free-list". (Maintain, in the hash table, a list of free entries,
initially empty. When deleting an entry, move it to the free-list. When a new
entry is wanted, take one from the free-list if available, else allocate it
from the pool.) This would make the memory used by a hash table proportional
to the maximum number of entries that it had ever held simultaneously, rather
than the number of insertions that had ever been performed.
In fact, using a free-list seems to be quite easy, with virtually no speed
penalty. A patch is attached, prepared in the manner of a Subversion patch.
(Please copy replies to dev@subversion.tigris.org, as I am not subscribed to
dev@apr.apache.org)
- Julian
[PATCH] APR hash table memory use
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> It would be possible to fix this in APR if people thought it worthwhile,
[...]
Well, in fact, it seems to be quite easy. Patch attached, prepared in the
manner of a Subversion patch. Does anyone here care to review or test it?
> It seems to me that the APR developers ought to be interested in doing
> this. Will I make the effort to take this to their mailing list? Don't
> know.
Having done a patch, I probably will.
- Julian