{"thread":{"id":"9257","subject":"[PATCH] really plug memory leaks in git-svnimport","startedAt":"2007-07-27T13:09:41Z","lastAt":"2007-07-27T13:09:41Z","messageCount":1,"participants":["Stefan Sperling"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"48797","messageId":"20070727130941.GB55326@ted.stsp.lan","threadId":"9257","inReplyTo":null,"subject":"[PATCH] really plug memory leaks in git-svnimport","fromName":"Stefan Sperling","fromEmail":"stsp@elego.de","sentAt":"2007-07-27T13:09:41Z","receivedAt":"2007-07-27T13:09:41Z","isPatch":true,"sender":{"key":"stsp@elego.de","avatar":"https://avatars.githubusercontent.com/u/9281333?v=4"},"body":"\nHello,\n\ntrying to convert the rather large  subversion repo of the DSLinux project\nI hit memory leaks in git-svnimport.\n\nThe repo has *lots* of files, a workspace is about 2GB in size.\nURLs: www.dslinux.org, svnweb at http://dslinux.gits.kiev.ua\n\nTrace showing the program running out of memory:\n\n#0  0x28285f57 in kill () from /lib/libc.so.6\n#1  0x28285ef6 in raise () from /lib/libc.so.6\n#2  0x282849bb in abort () from /lib/libc.so.6\n#3  0x283bd9ea in abort_on_pool_failure (retcode=12)\n    at subversion/libsvn_subr/pool.c:46\n#4  0x285c8f1f in apr_palloc (pool=0x2801a018, size=53840)\n    at memory/unix/apr_pools.c:618\n#5  0x28621519 in rep_read_contents (baton=0x2800d388, buf=0x28026018 \"\",\n    len=0xbfbfdb38) at subversion/libsvn_fs_fs/fs_fs.c:1881\n\n\nI know about the patch at http://marc.info/?l=git&m=114345884526971&w=2\nbut that is already applied to the version I have here and does not\nhelp because it does not really solve the core of the problem.\n\nThe following is rather lengthy, but I guess people applying\nmy patch want to understand what they're doing so I might just\nas well provide all the info upfront.\n\nThe core of the problem is that the subversion perl bindings\ndon't seem to suceed at abstracting dynamic memory handling\naway from perl scripts. Ripping out all explicit dynamic memory\nhandling from git-svnimport (done via the SVN::Pool API) does\nnot make the problem go away.\n\nI am still not sure whether the right place to fix this\nare the subversion perl bindings or the script.\nHandling dynamic memory explicitly in a scripting language\nis certainly weird. But I cannot get the subversion perl bindings\nfrom current subversion trunk to build on my system to try to find\nout if it is possible to fix the problem there\n(see http://svn.haxx.se/users/archive-2007-07/0784.shtml).\n\nSo I had to try to solve the problem in the script itself.\nI was able to fix the script up to the point where I was\nable to import the entire DSLinux repository in a single\ngo without the script running out of memory.\n\nTo understand the fix you need to understand how subversion\nhandles dynamic memory:\n\nBasically, every function in subversion that needs temporary\nscratch space gets handed a pointer to a \"memory pool\".\n\nIt can allocate memory from this pool and does not need to care\nabout freeing anything because only the scope that created a pool\nis responsible for destroying it.\n\nSo in C this looks a bit like:\n\nsvn_error_t* some_func(struct my_struct **result, apr_pool_t *pool)\n{\n\t/* allocate space for result from pool */\n\tstruct my_struct *res = apr_palloc(pool, sizeof(struct my_struct));\n\t\n\t/* compute result and pass pool further down if needed */\n\t\n\t...\n\n\t/* don't care about freeing the pool */\n\t*result = res;\n\treturn SVN_NO_ERROR;\n}\n\nThe caller will know when it does not need the result any longer\nand destroy the pool at that moment.\n\nLoops are interesting, since we can also \"clear\" a pool which does\nnot deallocate the memory it holds but marks the memory ready for re-use.\n\nSo with loops the following idiom is used:\n\n\tapr_pool_t *subpool = svn_pool_create(pool);\t\t\n\n\twhile (condition) {\n\t\tsvn_pool_clear(subpool);\n\n\t\t/* do stuff and pass subpool further down if needed */\n\n\t\t...\n\t}\n\t\n\tsvn_pool_destroy(subpool);\n\n\nSo how do these pools work in perl?\n\nWell, the SVN::Core man page states the following:\n\n  The perl bindings significantly simplify the usage of pools, while\n  still being manually adjustable.\n\nGreat. And what does mean exactly?\n\n  Functions requiring pool as the last argument (which are, almost all of\n  the subversion functions), the pool is optionally[sic]. The default pool\n  is used if it is omitted. If default pool is not set, a new root pool will\n  be created and set as default automatically when the first function\n  requiring a default pool is called.\n\nSo there's a \"default\" pool that is used if the caller does not\nspecify a $pool argument. We can take more control via the way\nwe create pools in perl:\n\n  Methods\n  \n  new ([$parent])\n     Create a new pool. The pool is a root pool if $parent is not sup-\n     plied.\n  \n  new_default ([$parent])\n     Create a new pool. The pool is a root pool if $parent is not sup-\n     plied.  Set the new pool as default pool.\n  \n  new_default_sub\n     Create a new subpool of the current default pool, and set the\n     resulting pool as new default pool.\n  \n  clear\n     Clear the pool.\n  \n  destroy\n     Destroy the pool. If the pool is the default pool, restore the pre-\n     vious default pool as default. This is normally called automati-\n     cally when the SVN::Pool object is no longer used and destroyed by\n     the perl garbage collector.\n  \nSo pools in perl get destroyed when the garbage collector runs.\nThe man page gives an example that implies that the garbage collector\nruns when the pool falls out of scope:\n\n  # create a root pool and set it as default pool for later use\n  my $pool = SVN::Pool->new_default;\n\n  sub something {\n      # create a subpool of the current default pool\n      my $pool = SVN::Pool->new_default_sub;\n      # some svn operations...\n\n      # $pool gets destroyed and the previous default pool\n      # is restored when $pool's lexical scope ends\n  }\n\ngit-svmimport uses the repository access (RA) layer of the subversion\nlibrary to talk to the repository. The SVN::Ra man page states\nthe following about the 'pool' argument of the SVN::Ra constructor:\n\n  The pool for the ra session to use, and also the member functions\n  will be called with this pool. Default to a newly created root\n  pool.\n\nThe SVN::Ra man page fails to mention what the SVN::Core man page implies.\nIf a function gets passed an explicit pool, or if we change the\ncurrent default pool, our pool will be used instead of the one\nspecified to the constructor of the RA layer!\n\nSo the patch below does the following:\n\n  * Create an explicit one-and-only root pool.\n\n  * Override the default pool SVN::RA is using with our root pool.\n    Since the connection to the repo must stay open during the whole\n    script, the RA layer will depend on the pool staying alive throughout\n    the whole program, so it might as well use the root pool.\n\n  * Closely follow the example in the SVN::Core man page.\n\n    Before calling a subversion function, create a subpool of our\n    root pool and make it the new default pool. Then call the\n    function without passing $pool argument. The function will use\n    our subpool anyway because we made it the current default pool.\n    The previous default pool will become default pool again when\n    the subpool falls out of scope.\n\n  * A major problem seemed to be that the script keeps creating\n    new root pools with\n\n      my $pool = SVN::Pool->new();\n    \n    inside a loop in *global* scope instead of using the loop idiom\n    described above. These pools are never, ever destroyed.\n\n    So create a subpool for the loop to use and clear (i.e. mark for\n    reuse, don't decallocate) the subpool at the start of the loop\n    instead of allocating new memory with each iteration.\n\n\nWith this patch, the script never exceeded the memory usage of firefox\nto a large extend while converting the DSLinux repo :-)\nI hope the patch will help even more stupid and ugly people switch to git.\n\n\ndiff --git a/git-svnimport.perl b/git-svnimport.perl\nindex b73d649..53526f4 100755\n--- a/git-svnimport.perl\n+++ b/git-svnimport.perl\n@@ -54,6 +54,7 @@ my $branch_name = $opt_b || \"branches\";\n my $project_name = $opt_P || \"\";\n $project_name = \"/\" . $project_name if ($project_name);\n my $repack_after = $opt_R || 1000;\n+my $root_pool = SVN::Pool->new_default;\n \n @ARGV == 1 or @ARGV == 2 or usage();\n \n@@ -132,7 +133,7 @@ sub conn {\n \tmy $auth = SVN::Core::auth_open ([SVN::Client::get_simple_provider,\n \t\t\t  SVN::Client::get_ssl_server_trust_file_provider,\n \t\t\t  SVN::Client::get_username_provider]);\n-\tmy $s = SVN::Ra->new(url => $repo, auth => $auth);\n+\tmy $s = SVN::Ra->new(url => $repo, auth => $auth, pool => $root_pool);\n \tdie \"SVN connection to $repo: $!\\n\" unless defined $s;\n \t$self->{'svn'} = $s;\n \t$self->{'repo'} = $repo;\n@@ -147,11 +148,10 @@ sub file {\n \n \tprint \"... $rev $path ...\\n\" if $opt_v;\n \tmy (undef, $properties);\n-\tmy $pool = SVN::Pool->new();\n \t$path =~ s#^/*##;\n+\tmy $subpool = SVN::Pool::new_default_sub;\n \teval { (undef, $properties)\n-\t\t   = $self->{'svn'}->get_file($path,$rev,$fh,$pool); };\n-\t$pool->clear;\n+\t\t   = $self->{'svn'}->get_file($path,$rev,$fh); };\n \tif($@) {\n \t\treturn undef if $@ =~ /Attempted to get checksum/;\n \t\tdie $@;\n@@ -185,6 +185,7 @@ sub ignore {\n \n \tprint \"... $rev $path ...\\n\" if $opt_v;\n \t$path =~ s#^/*##;\n+\tmy $subpool = SVN::Pool::new_default_sub;\n \tmy (undef,undef,$properties)\n \t    = $self->{'svn'}->get_dir($path,$rev,undef);\n \tif (exists $properties->{'svn:ignore'}) {\n@@ -202,6 +203,7 @@ sub ignore {\n sub dir_list {\n \tmy($self,$path,$rev) = @_;\n \t$path =~ s#^/*##;\n+\tmy $subpool = SVN::Pool::new_default_sub;\n \tmy ($dirents,undef,$properties)\n \t    = $self->{'svn'}->get_dir($path,$rev,undef);\n \treturn $dirents;\n@@ -358,10 +360,9 @@ open BRANCHES,\">>\", \"$git_dir/svn2git\";\n \n sub node_kind($$) {\n \tmy ($svnpath, $revision) = @_;\n-\tmy $pool=SVN::Pool->new;\n \t$svnpath =~ s#^/*##;\n-\tmy $kind = $svn->{'svn'}->check_path($svnpath,$revision,$pool);\n-\t$pool->clear;\n+\tmy $subpool = SVN::Pool::new_default_sub;\n+\tmy $kind = $svn->{'svn'}->check_path($svnpath,$revision);\n \treturn $kind;\n }\n \n@@ -889,7 +890,7 @@ sub commit_all {\n \t# Recursive use of the SVN connection does not work\n \tlocal $svn = $svn2;\n \n-\tmy ($changed_paths, $revision, $author, $date, $message, $pool) = @_;\n+\tmy ($changed_paths, $revision, $author, $date, $message) = @_;\n \tmy %p;\n \twhile(my($path,$action) = each %$changed_paths) {\n \t\t$p{$path} = [ $action->action,$action->copyfrom_path, $action->copyfrom_rev, $path ];\n@@ -925,14 +926,14 @@ print \"Processing from $current_rev to $opt_l ...\\n\" if $opt_v;\n my $from_rev;\n my $to_rev = $current_rev - 1;\n \n+my $subpool = SVN::Pool::new_default_sub;\n while ($to_rev < $opt_l) {\n+\t$subpool->clear;\n \t$from_rev = $to_rev + 1;\n \t$to_rev = $from_rev + $repack_after;\n \t$to_rev = $opt_l if $opt_l < $to_rev;\n \tprint \"Fetching from $from_rev to $to_rev ...\\n\" if $opt_v;\n-\tmy $pool=SVN::Pool->new;\n-\t$svn->{'svn'}->get_log(\"/\",$from_rev,$to_rev,0,1,1,\\&commit_all,$pool);\n-\t$pool->clear;\n+\t$svn->{'svn'}->get_log(\"/\",$from_rev,$to_rev,0,1,1,\\&commit_all);\n \tmy $pid = fork();\n \tdie \"Fork: $!\\n\" unless defined $pid;\n \tunless($pid) {\n"}]}