You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2012/11/12 12:04:48 UTC
Re: svn commit: r1406366 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
On Wed, Nov 7, 2012 at 2:03 AM, <cm...@apache.org> wrote:
> Author: cmpilato
> Date: Tue Nov 6 22:03:04 2012
> New Revision: 1406366
>
> URL: http://svn.apache.org/viewvc?rev=1406366&view=rev
> Log:
> Teach ra_serf to more carefully track the PROPFIND requests it issues
> for directories, especially the completion thereof, and to
> opportunistically close directory batons that no longer need to be
> opened. Prior to this change, directories in which no files had their
> contents fetched from the server were held open until the end of the
> editor drive, which caused a nasty accumulation of allocated tracking
> structures.
>
> I'll be so bold as to claim that this fixed issue #4194 ("serf memory
> leak on checkout").
>
> * subversion/libsvn_ra_serf/update.c
> (report_dir_t): Add 'done_dir_propfinds' and 'active_dir_propfinds' lists.
> (report_context_t): Add 'closed_root' flag.
> (maybe_close_dir_chain): New helper function, abstracted from logic
> in finish_report(), which encapsulates the "see if we can close a
> directory and its parents opportunistically" logic.
> (handle_propchange_only, handle_local_content): Now call
> maybe_close_dir_chain(), too.
> (end_report): Track completed directory PROPFINDs in the new
> 'done_dir_propfinds' list, now, and also populate the new
> 'active_dir_propfinds' list.
> (finish_report): Now also monitor the completed directory PROPFINDs,
> using maybe_close_dir_chain() to close directories sooner rather
> than later.
>
[...]
> /* Open the file associated with INFO for editing, pass along any
> propchanges we've recorded for it, and then close the file. */
> static svn_error_t *
> @@ -1189,6 +1234,7 @@ handle_propchange_only(report_info_t *in
> svn_pool_destroy(info->pool);
>
> info->dir->ref_count--;
> + SVN_ERR(maybe_close_dir_chain(info->dir));
>
> return SVN_NO_ERROR;
> }
> @@ -1213,6 +1259,7 @@ handle_local_content(report_info_t *info
> svn_pool_destroy(info->pool);
>
> info->dir->ref_count--;
> + SVN_ERR(maybe_close_dir_chain(info->dir));
>
Mike,
This change seems to cause problem with access to freed memory: list
item also allocated in dir pool and finish_report() accesses it after
calling to handle_local_content().
--
Ivan Zhakov
Re: svn commit: r1406366 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, Nov 12, 2012 at 5:59 PM, C. Michael Pilato <cm...@collab.net> wrote:
> On 11/12/2012 08:22 AM, Ivan Zhakov wrote:
>> I'm also get similar crash on Windows and MacOS. Commenting call to
>> maybe_close_dir() in handle_propchange_only()/handle_local_content()
>> fixes them, but increase memory usage.
>
> Those two call sites were "just in case" sorts of additions. Obviously,
> they didn't trigger SEGFAULTs on my end, or I wouldn't have committed them.
> But I didn't have any reason to believe they were critical to the
> memory-reduction work, so let's just remove them. (I'll do so.)
>
Without these calls memory usage is very high: checkout of
subversion/branches consumes about 200mb of memory (then crashes btw).
So we need these calls, but probably in some other place.
--
Ivan Zhakov
Re: svn commit: r1406366 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Philip Martin <ph...@wandisco.com>.
"C. Michael Pilato" <cm...@collab.net> writes:
> On 11/12/2012 08:22 AM, Ivan Zhakov wrote:
>> I'm also get similar crash on Windows and MacOS. Commenting call to
>> maybe_close_dir() in handle_propchange_only()/handle_local_content()
>> fixes them, but increase memory usage.
>
> Those two call sites were "just in case" sorts of additions. Obviously,
> they didn't trigger SEGFAULTs on my end, or I wouldn't have committed them.
> But I didn't have any reason to believe they were critical to the
> memory-reduction work, so let's just remove them. (I'll do so.)
I'm still seeing the same SEGV with r1408311:
#0 0x00007ffff5b22daf in finish_report (report_baton=0x7ffff7e42cc0,
pool=0x7ffff7e40028) at ../src/subversion/libsvn_ra_serf/update.c:2636
#1 0x00007ffff789ac6d in svn_wc_crawl_revisions5 (wc_ctx=0x7ffff32fc6c0,
local_abspath=0x7ffff7e40168 "/home/pm/sw/subversion/obj/wc",
reporter=0x7ffff5d384e0, report_baton=0x7ffff7e42cc0, restore_files=1,
depth=svn_depth_unknown, honor_depth_exclude=1,
depth_compatibility_trick=0, use_commit_times=0,
cancel_func=0x416b43 <svn_cl__check_cancel>, cancel_baton=0x0,
notify_func=0x41c4db <notify>, notify_baton=0x7ffff32fc9b0,
scratch_pool=0x7ffff7e40028)
at ../src/subversion/libsvn_wc/adm_crawler.c:858
#2 0x00007ffff7bc800d in update_internal (result_rev=0x0,
local_abspath=0x7ffff7e40168 "/home/pm/sw/subversion/obj/wc",
anchor_abspath=0x7ffff7e418a0 "/home/pm/sw/subversion/obj/wc",
revision=0x7fffffffe050, depth=svn_depth_unknown, depth_is_sticky=0,
ignore_externals=0, allow_unver_obstructions=0, adds_as_modification=1,
timestamp_sleep=0x7fffffffe15c, notify_summary=1, ctx=0x7ffff32fc608,
pool=0x7ffff7e40028) at ../src/subversion/libsvn_client/update.c:427
#3 0x00007ffff7bc8597 in svn_client__update_internal (result_rev=0x0,
local_abspath=0x7ffff7e40168 "/home/pm/sw/subversion/obj/wc",
revision=0x7fffffffe2b0, depth=svn_depth_unknown, depth_is_sticky=1,
ignore_externals=0, allow_unver_obstructions=0, adds_as_modification=1,
make_parents=0, innerupdate=0, timestamp_sleep=0x7fffffffe15c,
ctx=0x7ffff32fc608, pool=0x7ffff7e40028)
at ../src/subversion/libsvn_client/update.c:569
#4 0x00007ffff7b6ee45 in svn_client__checkout_internal (result_rev=0x0,
url=0x7ffff32fd638 "http://localhost:8080/repos/asf/subversion/trunk",
local_abspath=0x7ffff7e40168 "/home/pm/sw/subversion/obj/wc",
peg_revision=0x7fffffffe2a0, revision=0x7fffffffe2b0,
depth=svn_depth_unknown, ignore_externals=0, allow_unver_obstructions=0,
timestamp_sleep=0x0, ctx=0x7ffff32fc608, pool=0x7ffff7e40028)
at ../src/subversion/libsvn_client/checkout.c:165
#5 0x00007ffff7b6ef5a in svn_client_checkout3 (result_rev=0x0,
URL=0x7ffff32fd638 "http://localhost:8080/repos/asf/subversion/trunk",
path=0x7ffff32fd6e0 "wc", peg_revision=0x7fffffffe2a0,
revision=0x7fffffffe2b0, depth=svn_depth_unknown, ignore_externals=0,
allow_unver_obstructions=0, ctx=0x7ffff32fc608, pool=0x7ffff7e40028)
at ../src/subversion/libsvn_client/checkout.c:205
#6 0x0000000000409615 in svn_cl__checkout (os=0x7ffff7feb280,
baton=0x7fffffffe4b0, pool=0x7ffff7feb028)
at ../src/subversion/svn/checkout-cmd.c:161
#7 0x00000000004197e4 in sub_main (argc=4, argv=0x7fffffffe958,
pool=0x7ffff7feb028) at ../src/subversion/svn/main.c:2735
#8 0x00000000004199cd in main (argc=4, argv=0x7fffffffe958)
at ../src/subversion/svn/main.c:2790
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1406366 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/12/2012 08:22 AM, Ivan Zhakov wrote:
> I'm also get similar crash on Windows and MacOS. Commenting call to
> maybe_close_dir() in handle_propchange_only()/handle_local_content()
> fixes them, but increase memory usage.
Those two call sites were "just in case" sorts of additions. Obviously,
they didn't trigger SEGFAULTs on my end, or I wouldn't have committed them.
But I didn't have any reason to believe they were critical to the
memory-reduction work, so let's just remove them. (I'll do so.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1406366 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, Nov 12, 2012 at 4:29 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> On Wed, Nov 7, 2012 at 2:03 AM, <cm...@apache.org> wrote:
>>> /* Open the file associated with INFO for editing, pass along any
>>> propchanges we've recorded for it, and then close the file. */
>>> static svn_error_t *
>>> @@ -1189,6 +1234,7 @@ handle_propchange_only(report_info_t *in
>>> svn_pool_destroy(info->pool);
>>>
>>> info->dir->ref_count--;
>>> + SVN_ERR(maybe_close_dir_chain(info->dir));
>>>
>>> return SVN_NO_ERROR;
>>> }
>>> @@ -1213,6 +1259,7 @@ handle_local_content(report_info_t *info
>>> svn_pool_destroy(info->pool);
>>>
>>> info->dir->ref_count--;
>>> + SVN_ERR(maybe_close_dir_chain(info->dir));
>>>
>> Mike,
>>
>> This change seems to cause problem with access to freed memory: list
>> item also allocated in dir pool and finish_report() accesses it after
>> calling to handle_local_content().
>
> I get SEGV when using a 1.8 client to checkout from a 1.7 server,
> valgrind reports:
>
> ==19159== Invalid read of size 8
> ==19159== at 0x6EF7E1F: finish_report (update.c:2637)
> ==19159== by 0x50CCC6C: svn_wc_crawl_revisions5 (adm_crawler.c:858)
> ==19159== by 0x4EA000C: update_internal (update.c:427)
> ==19159== by 0x4EA0596: svn_client__update_internal (update.c:569)
> ==19159== by 0x4E46E44: svn_client__checkout_internal (checkout.c:165)
> ==19159== by 0x4E46F59: svn_client_checkout3 (checkout.c:205)
> ==19159== by 0x409614: svn_cl__checkout (checkout-cmd.c:161)
> ==19159== by 0x4197E3: sub_main (main.c:2735)
> ==19159== by 0x4199CC: main (main.c:2790)
> ==19159== Address 0xa3b1bd0 is not stack'd, malloc'd or (recently) free'd
>
I'm also get similar crash on Windows and MacOS. Commenting call to
maybe_close_dir() in handle_propchange_only()/handle_local_content()
fixes them, but increase memory usage.
--
Ivan Zhakov
Re: svn commit: r1406366 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> On Wed, Nov 7, 2012 at 2:03 AM, <cm...@apache.org> wrote:
>> /* Open the file associated with INFO for editing, pass along any
>> propchanges we've recorded for it, and then close the file. */
>> static svn_error_t *
>> @@ -1189,6 +1234,7 @@ handle_propchange_only(report_info_t *in
>> svn_pool_destroy(info->pool);
>>
>> info->dir->ref_count--;
>> + SVN_ERR(maybe_close_dir_chain(info->dir));
>>
>> return SVN_NO_ERROR;
>> }
>> @@ -1213,6 +1259,7 @@ handle_local_content(report_info_t *info
>> svn_pool_destroy(info->pool);
>>
>> info->dir->ref_count--;
>> + SVN_ERR(maybe_close_dir_chain(info->dir));
>>
> Mike,
>
> This change seems to cause problem with access to freed memory: list
> item also allocated in dir pool and finish_report() accesses it after
> calling to handle_local_content().
I get SEGV when using a 1.8 client to checkout from a 1.7 server,
valgrind reports:
==19159== Invalid read of size 8
==19159== at 0x6EF7E1F: finish_report (update.c:2637)
==19159== by 0x50CCC6C: svn_wc_crawl_revisions5 (adm_crawler.c:858)
==19159== by 0x4EA000C: update_internal (update.c:427)
==19159== by 0x4EA0596: svn_client__update_internal (update.c:569)
==19159== by 0x4E46E44: svn_client__checkout_internal (checkout.c:165)
==19159== by 0x4E46F59: svn_client_checkout3 (checkout.c:205)
==19159== by 0x409614: svn_cl__checkout (checkout-cmd.c:161)
==19159== by 0x4197E3: sub_main (main.c:2735)
==19159== by 0x4199CC: main (main.c:2790)
==19159== Address 0xa3b1bd0 is not stack'd, malloc'd or (recently) free'd
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download