On 25.02.2015 18:07, Ivan Zhakov wrote:
> On 25 February 2015 at 17:40, Branko Čibej <brane_at_wandisco.com> wrote:
>> On 25.02.2015 15:23, ivan_at_apache.org wrote:
>>> Author: ivan
>>> Date: Wed Feb 25 14:23:00 2015
>>> New Revision: 1662222
>>>
>>> URL: http://svn.apache.org/r1662222
>>> Log:
>>> On the 'reuse-ra-session' branch: Resolve memory leak.
>>>
>>> * subversion/libsvn_client/ra_cache.c
>>> (): Include "svn_pools.h".
>>>
>>> (cache_entry_t): Add SESSION_POOL member to have an option to destroy
>>> CACHE_ENTRY structure itself.
>>>
>>> (close_ra_session, remove_inactive_entry): Destroy SESSION_POOL
>>> instead of closing RA session -- we have other resources allocated
>>> for RA session, like RA callback baton, cache_entry etc.
>>>
>>> (open_new_session): New helper, factored out from
>>> svn_client__ra_cache_open_session().
>>>
>>> (svn_client__ra_cache_open_session): Create separate subpool for
>>> each CACHE_ENTRY and destroy it on error.
>> So ... from this point on, we create *two* subpools for each session
>> (svn_ra_open creates one, and with the above change, we create the
>> other). That seems wasteful.
>>
> That's correct. But RA session relatively expensive object (it
> maintain TCP connections etc) and one extra subpool should not be big
> issue IMO.
>
>> I'd prefer to have a way to tell svn_ra_open to /not/ create a subpool
>> for the session, perhaps by exposing a private ra-open function where we
>> can pass in the actual session pool, and to keep svn_ra__close.
>>
> We still need separate subpool to allocate CACHE_ENTRY itself,
> callbacks baton etc. And inventing private API to just save one
> subpool looks like a bad idea for me.
>
> I'd prefer to drop SESSPOOL in ra-loader.c if we consider that two
> subpools per RA session is serious problem: it was added in r984280
> when HTTP redirects was implemented. I think that the caller should be
> responsible to clear pool if it's going to follow redirects.
Since r1664078 and related changes, the session really does need its own
pool. Those changes also reduced the overhead allocation from the
session pool itself, using a scratch pool for the temporary bits that's
destroyed before the new session is returned to the caller.
-- Brane
Received on 2015-03-11 08:42:23 CET