[PATCH] Reintroduce svn pools to solve the memory leak.

[PATCH] Reintroduce svn pools to solve the memory leak.

[Santi Béjar]

On 3/24/06, Santi Béjar <sbejar@gmail.com> wrote:
> Jan-Benedict Glaw <jbglaw@lug-owl.de> writes:
>
> > On Wed, 2006-03-22 14:33:37 +0100, Jan-Benedict Glaw <jbglaw@lug-owl.de> wrote:
> >
> > Since it seems nobody looked at the GCC import run (which means to use
> > the svnimport), I ran it again, under strace control:
> >
> >> GCC
> >> ~~~
> >> $ /home/jbglaw/bin/git svnimport -C gcc -v svn://gcc.gnu.org/svn/gcc
> >
> >> Committed change 3936:/ 1993-03-31 05:44:03)
> >> Commit ID ceff85145f8671fb2a9d826a761cedc2a507bd1e
> >> Writing to refs/heads/origin
> >> DONE: 3936 origin ceff85145f8671fb2a9d826a761cedc2a507bd1e
> >> ... 3937 trunk/gcc/final.c ...
> >> Can't fork at /home/jbglaw/bin/git-svnimport line 379.
> >
>
> I have the same (?) problem with one of my svn repository. It worked
> before (I've redone the import with the -r flag), so I bisected it.
> The problematic commit seems to be:
>
> diff-tree 4802426... (from 525c0d7...)
> Author: Karl  Hasselström <kha@treskal.com>
> Date:   Sun Feb 26 06:11:27 2006 +0100
>
>     svnimport: Convert executable flag
>
>     Convert the svn:executable property to file mode 755 when converting
>     an SVN repository to GIT.
>
>     Signed-off-by: Karl Hasselström <kha@treskal.com>
>     Signed-off-by: Junio C Hamano <junkio@cox.net>
>
> :100755 100755 ee2940f... 6603b96... M  git-svnimport.perl
>
> I think it has a memory leak, it used up to 140m of memory.
>
> $ git reset --hard 4802426^
> $ time ../git-svnimport.perl file:///path/
> Use of uninitialized value in string eq at ../git-svnimport.perl line 463.
> Use of uninitialized value in substitution (s///) at ../git-svnimport.perl line 466.
> real    0m55.801s
> user    0m30.578s
> sys     0m23.084s
>
> $ git reset --hard 4802426
> $ time ../git-svnimport.perl file:///path/
> Use of uninitialized value in string eq at ../git-svnimport.perl line 463.
> Use of uninitialized value in substitution (s///) at ../git-svnimport.perl line 466.
> Can't fork at /home/santi/usr/src/scm/git/git-svnimport.perl line 331.
> real    6m2.163s
> user    0m20.332s
> sys     0m50.180s
>
> and it didn't finished. Hope it helps.

And this patch fixes my problems.

---

Introduced in 4802426.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 git-svnimport.perl |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-svnimport.perl b/git-svnimport.perl
index 639aa41..f2cf062 100755
--- a/git-svnimport.perl
+++ b/git-svnimport.perl
@@ -135,8 +135,10 @@

        print "... $rev $path ...\n" if $opt_v;
        my (undef, $properties);
+       my $pool = SVN::Pool->new();
        eval { (undef, $properties)
-                  = $self->{'svn'}->get_file($path,$rev,$fh); };
+                  = $self->{'svn'}->get_file($path,$rev,$fh,$pool); };
+       $pool->clear;
        if($@) {
                return undef if $@ =~ /Attempted to get checksum/;
                die $@;

Re: [PATCH] Reintroduce svn pools to solve the memory leak.

[Junio C Hamano]

"Santi Béjar" <sbejar@gmail.com> writes:

> On 3/24/06, Santi Béjar <sbejar@gmail.com> wrote:
>> Jan-Benedict Glaw <jbglaw@lug-owl.de> writes:
>>
>> diff-tree 4802426... (from 525c0d7...)
>> Author: Karl  Hasselström <kha@treskal.com>
>> Date:   Sun Feb 26 06:11:27 2006 +0100
>>
>>     svnimport: Convert executable flag
>>
>>     Convert the svn:executable property to file mode 755 when converting
>>     an SVN repository to GIT.
>>
>>     Signed-off-by: Karl Hasselström <kha@treskal.com>
>>     Signed-off-by: Junio C Hamano <junkio@cox.net>
>>
>> :100755 100755 ee2940f... 6603b96... M  git-svnimport.perl
>
> And this patch fixes my problems.

Jan-Benedict, thanks for pinpointing the regression, and Santi,
thanks for the patch.

I should have looked a bit more closely when applying the patch
-- it is clear that the patch is doing more than what its log
says.  My fault.

Karl, were there other reasons you needed to disable the pool
here (maybe to work around a problem with incompatible version
of SVN module)?  I see some other uses of SVN::Pool still there
in the code, so I am assuming this was a simple typo, but just
in case...

Re: [PATCH] Reintroduce svn pools to solve the memory leak.

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

On Mon, 2006-03-27 10:16:58 -0800, Junio C Hamano <junkio@cox.net> wrote:
> "Santi Béjar" <sbejar@gmail.com> writes:
> > On 3/24/06, Santi Béjar <sbejar@gmail.com> wrote:
> >> Jan-Benedict Glaw <jbglaw@lug-owl.de> writes:
> >>
> >> diff-tree 4802426... (from 525c0d7...)
> >> Author: Karl  Hasselström <kha@treskal.com>
> >> Date:   Sun Feb 26 06:11:27 2006 +0100
> >>
> >>     svnimport: Convert executable flag
> >>
> >>     Convert the svn:executable property to file mode 755 when converting
> >>     an SVN repository to GIT.
> >>
> >>     Signed-off-by: Karl Hasselström <kha@treskal.com>
> >>     Signed-off-by: Junio C Hamano <junkio@cox.net>
> >>
> >> :100755 100755 ee2940f... 6603b96... M  git-svnimport.perl
> >
> > And this patch fixes my problems.
> 
> Jan-Benedict, thanks for pinpointing the regression, and Santi,
> thanks for the patch.

I'm currently running another test with GCC: this patch cuts down
memory consumption to less than 1/10 of the previous state (VSZ) and
even more for RSS (my system is quite loaded...)

MfG, JBG

-- 
Jan-Benedict Glaw       jbglaw@lug-owl.de    . +49-172-7608481             _ O _
"Eine Freie Meinung in  einem Freien Kopf    | Gegen Zensur | Gegen Krieg  _ _ O
 für einen Freien Staat voll Freier Bürger"  | im Internet! |   im Irak!   O O O
ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Reintroduce svn pools to solve the memory leak.

[Karl Hasselström]

On 2006-03-27 10:16:58 -0800, Junio C Hamano wrote:

> Karl, were there other reasons you needed to disable the pool here
> (maybe to work around a problem with incompatible version of SVN
> module)? I see some other uses of SVN::Pool still there in the code,
> so I am assuming this was a simple typo, but just in case...

No, it's just a simple mistake (the mistake being me not realizing why
an explicit pool was needed, and simply dropping it when things worked
fine without it).

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle