{"thread":{"id":"40395","subject":"[PATCH] git-svn: make batch mode optional for git-cat-file","startedAt":"2015-09-21T13:51:38Z","lastAt":"2015-10-11T12:31:57Z","messageCount":10,"participants":["Victor Leschuk","Junio C Hamano","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270396","messageId":"1442843498-22908-1-git-send-email-vleschuk@accesssoftek.com","threadId":"40395","inReplyTo":null,"subject":"[PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-09-21T13:51:38Z","receivedAt":"2015-09-21T13:51:38Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"\nSigned-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n---\n git-svn.perl |  1 +\n perl/Git.pm  | 41 ++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 41 insertions(+), 1 deletion(-)\n\n\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 36f7240..b793c26 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' => \\$Git::SVN::_follow_parent,\n \t\t'use-log-author' => \\$Git::SVN::_use_log_author,\n \t\t'add-author-from' => \\$Git::SVN::_add_author_from,\n \t\t'localtime' => \\$Git::SVN::_localtime,\n+\t\t'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },\n \t\t%remote_opts );\n \n my ($_trunk, @_tags, @_branches, $_stdlayout);\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 19ef081..69e5293 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);\n use Time::Local qw(timegm);\n }\n \n+our $no_cat_file_batch = 0;\n \n =head1 CONSTRUCTORS\n \n@@ -1012,6 +1013,10 @@ returns the number of bytes printed.\n =cut\n \n sub cat_blob {\n+\t(1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);\n+}\n+\n+sub _cat_blob_batch {\n \tmy ($self, $sha1, $fh) = @_;\n \n \t$self->_open_cat_blob_if_needed();\n@@ -1072,7 +1077,7 @@ sub cat_blob {\n sub _open_cat_blob_if_needed {\n \tmy ($self) = @_;\n \n-\treturn if defined($self->{cat_blob_pid});\n+\treturn if ( defined($self->{cat_blob_pid}) || 1 == $no_cat_file_batch );\n \n \t($self->{cat_blob_pid}, $self->{cat_blob_in},\n \t $self->{cat_blob_out}, $self->{cat_blob_ctx}) =\n@@ -1090,6 +1095,40 @@ sub _close_cat_blob {\n \tdelete @$self{@vars};\n }\n \n+sub _cat_blob_cmd {\n+\tmy ($self, $sha1, $fh) = @_;\n+\n+\tmy $size = $self->command_oneline('cat-file', '-s', $sha1);\n+\n+\tif (!defined $size) {\n+\t\tcarp \"cat-file couldn't detect object size\";\n+\t\treturn -1;\n+\t}\n+\n+\tmy ($in, $c) = $self->command_output_pipe('cat-file', 'blob', $sha1);\n+\n+\tmy $blob;\n+\tmy $bytesLeft = $size;\n+\n+\twhile (1) {\n+\t\tlast unless $bytesLeft;\n+\n+\t\tmy $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;\n+\t\tmy $read = read($in, $blob, $bytesToRead);\n+\t\tunless (defined($read)) {\n+\t\t\t$self->command_close_pipe($in, $c);\n+\t\t\tthrow Error::Simple(\"in pipe went bad\");\n+\t\t}\n+\t\tunless (print $fh $blob) {\n+\t\t\t$self->command_close_pipe($in, $c);\n+\t\t\tthrow Error::Simple(\"couldn't write to passed in filehandle\");\n+\t\t}\n+\t\t$bytesLeft -= $read;\n+\t}\n+\n+\t$self->command_close_pipe($in, $c);\n+\treturn $size;\n+}\n \n =item credential_read( FILEHANDLE )\n \n"},{"id":"270417","messageId":"xmqqeghraauu.fsf@gitster.mtv.corp.google.com","threadId":"40395","inReplyTo":"1442843498-22908-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-21T18:25:13Z","receivedAt":"2015-09-21T18:25:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victor Leschuk <vleschuk@gmail.com> writes:\n\n> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n\nBefore the S-o-b line is a good place to explain why this is a good\nchange to have.  Please use it.\n\n>  git-svn.perl |  1 +\n>  perl/Git.pm  | 41 ++++++++++++++++++++++++++++++++++++++++-\n>  2 files changed, 41 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-svn.perl b/git-svn.perl\n> index 36f7240..b793c26 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' => \\$Git::SVN::_follow_parent,\n>  \t\t'use-log-author' => \\$Git::SVN::_use_log_author,\n>  \t\t'add-author-from' => \\$Git::SVN::_add_author_from,\n>  \t\t'localtime' => \\$Git::SVN::_localtime,\n> +\t\t'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },\n\nAn option whose name begins with no- looks somewhat strange.  You\ncan even say --no-no-cat-file-batch from the command line, I\nsuspect.\n\nWhy not give an option 'cat-file-batch' that sets the variable\n$Git::cat_file_batch to false, and initialize the variable to true\nto keep existing users who do not pass the option happy?\n\n>  \t\t%remote_opts );\n>  \n>  my ($_trunk, @_tags, @_branches, $_stdlayout);\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 19ef081..69e5293 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);\n>  use Time::Local qw(timegm);\n>  }\n>  \n> +our $no_cat_file_batch = 0;\n>  \n>  =head1 CONSTRUCTORS\n>  \n> @@ -1012,6 +1013,10 @@ returns the number of bytes printed.\n>  =cut\n>  \n>  sub cat_blob {\n> +\t(1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);\n\nDiscard \"1 ==\" here.  You are clearly using the variable as a\nboolean, so writing this as\n\n\t$cat_file_batch ? _cat_blob_batch(@_) : _cat_blob_cmd(@_);\n\nor better yet\n\n\tif ($cat_file_batch) {\n        \t_cat_blob_batch(@_);\n\t} else {\n        \t_cat_blob_cmd(@_);\n\t}\n\nwould be more natural.\n\n> +}\n> +\n> +sub _cat_blob_batch {\n>  \tmy ($self, $sha1, $fh) = @_;\n>  \n>  \t$self->_open_cat_blob_if_needed();\n> @@ -1072,7 +1077,7 @@ sub cat_blob {\n>  sub _open_cat_blob_if_needed {\n>  \tmy ($self) = @_;\n>  \n> -\treturn if defined($self->{cat_blob_pid});\n> +\treturn if ( defined($self->{cat_blob_pid}) || 1 == $no_cat_file_batch );\n\nLikewise.\n\n\treturn if (!$cat_file_batch);\n\treturn if defined($self->{cat_blob_pid});\n\n> +sub _cat_blob_cmd {\n> +\tmy ($self, $sha1, $fh) = @_;\n> +...\n\nThe biggest thing that is missing from this patch is the explanation\nof why this is a good thing to do.  The batch interface was invented\nbecause people found that it was wasteful to spawn a new cat-file\nprocess every time the contents of a blob is needed and wanted to\navoid it, and this new feature gives the user a way to tell Git to\ndo things in a \"wasteful\" way, so there must be a reason why the\nuser would want to use the \"wasteful\" way, perhaps work around some\nother issue.  Without explaining that in the documentation what that\nissue is, i.e. telling users who reads \"git svn --help\" when and why\nthe option might help them, nobody would use the feature to benefit\nfrom it.\n\nI wonder if \"cat-file --batch\" is leaky and bloats after running for\na while.  If that is the case, I have to wonder if \"never do batch\"\nlike this patch does is a sensible way forward.  Instead, \"recycle\nand renew the process after running it for N requests\" (and ideally\nauto-adjust that N without being told by the user) might be a better\nway to do what you are trying to achieve, but as I already said, I\ncannot read the motivation behind this change that is not explained,\nso...\n"},{"id":"270427","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9D6@mail.accesssoftek.com","threadId":"40395","inReplyTo":"xmqqeghraauu.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-09-21T22:03:46Z","receivedAt":"2015-09-21T22:03:46Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello Junio,\n\nthanks for your review. First of all I'd like to apologize for sending the patch without description. Actually I was in a hurry and sent it by accident: I planned to edit the mail before sending... \n\nHere is the detailed description: \n\nLast week we had a quick discussion in this mailing list: http://thread.gmane.org/gmane.comp.version-control.git/278021 .\n\nThe thing is that git-cat-file keeps growing during work when running in \"batch\" mode. See the figure attached: it is for cloning a rather small repo (1 hour to clone about ~14000 revisions). However the clone of a large repo (~280000 revisions) took about 2 weeks and git-cat-file has outgrown the parent perl process several times (git-cat-file - ~3-4Gb, perl - 400Mb).\n\nWhat was done:\n * I have run it under valgrind and mtrace and haven't found any memory leaks\n * Found the source of most number of memory reallocations (batch_object_write() function (strbuf_expand -> realloc)) - tried to make the streambuf object static and avoid reallocs - didn't help\n * Tried preloading other allocators than standard glibc - no significant difference\n\nAfter that I replaced the batch mode with separate cat-file calls for each blob and it didn't have any impact on clone performance on real code repositories. However I created a fake test repo with large number of small files (~10 bytes each): here is how I created it https://bitbucket.org/vleschuk/svngenrepo\n\nAnd on this artificial test repo it really slowed down the process. So I decided to suggest to make the batch mode optional to let the user \"tune\" the process and created a patch for this. \n\nAs for your code-style notes, I agree with them, will adjust the code.\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Junio C Hamano [jch2355@gmail.com] On Behalf Of Junio C Hamano [gitster@pobox.com]\nSent: Monday, September 21, 2015 11:25 AM\nTo: Victor Leschuk\nCc: git@vger.kernel.org; Victor Leschuk\nSubject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file\n\nVictor Leschuk <vleschuk@gmail.com> writes:\n\n> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n\nBefore the S-o-b line is a good place to explain why this is a good\nchange to have.  Please use it.\n\n>  git-svn.perl |  1 +\n>  perl/Git.pm  | 41 ++++++++++++++++++++++++++++++++++++++++-\n>  2 files changed, 41 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-svn.perl b/git-svn.perl\n> index 36f7240..b793c26 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' => \\$Git::SVN::_follow_parent,\n>               'use-log-author' => \\$Git::SVN::_use_log_author,\n>               'add-author-from' => \\$Git::SVN::_add_author_from,\n>               'localtime' => \\$Git::SVN::_localtime,\n> +             'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },\n\nAn option whose name begins with no- looks somewhat strange.  You\ncan even say --no-no-cat-file-batch from the command line, I\nsuspect.\n\nWhy not give an option 'cat-file-batch' that sets the variable\n$Git::cat_file_batch to false, and initialize the variable to true\nto keep existing users who do not pass the option happy?\n\n>               %remote_opts );\n>\n>  my ($_trunk, @_tags, @_branches, $_stdlayout);\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 19ef081..69e5293 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);\n>  use Time::Local qw(timegm);\n>  }\n>\n> +our $no_cat_file_batch = 0;\n>\n>  =head1 CONSTRUCTORS\n>\n> @@ -1012,6 +1013,10 @@ returns the number of bytes printed.\n>  =cut\n>\n>  sub cat_blob {\n> +     (1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);\n\nDiscard \"1 ==\" here.  You are clearly using the variable as a\nboolean, so writing this as\n\n        $cat_file_batch ? _cat_blob_batch(@_) : _cat_blob_cmd(@_);\n\nor better yet\n\n        if ($cat_file_batch) {\n                _cat_blob_batch(@_);\n        } else {\n                _cat_blob_cmd(@_);\n        }\n\nwould be more natural.\n\n> +}\n> +\n> +sub _cat_blob_batch {\n>       my ($self, $sha1, $fh) = @_;\n>\n>       $self->_open_cat_blob_if_needed();\n> @@ -1072,7 +1077,7 @@ sub cat_blob {\n>  sub _open_cat_blob_if_needed {\n>       my ($self) = @_;\n>\n> -     return if defined($self->{cat_blob_pid});\n> +     return if ( defined($self->{cat_blob_pid}) || 1 == $no_cat_file_batch );\n\nLikewise.\n\n        return if (!$cat_file_batch);\n        return if defined($self->{cat_blob_pid});\n\n> +sub _cat_blob_cmd {\n> +     my ($self, $sha1, $fh) = @_;\n> +...\n\nThe biggest thing that is missing from this patch is the explanation\nof why this is a good thing to do.  The batch interface was invented\nbecause people found that it was wasteful to spawn a new cat-file\nprocess every time the contents of a blob is needed and wanted to\navoid it, and this new feature gives the user a way to tell Git to\ndo things in a \"wasteful\" way, so there must be a reason why the\nuser would want to use the \"wasteful\" way, perhaps work around some\nother issue.  Without explaining that in the documentation what that\nissue is, i.e. telling users who reads \"git svn --help\" when and why\nthe option might help them, nobody would use the feature to benefit\nfrom it.\n\nI wonder if \"cat-file --batch\" is leaky and bloats after running for\na while.  If that is the case, I have to wonder if \"never do batch\"\nlike this patch does is a sensible way forward.  Instead, \"recycle\nand renew the process after running it for N requests\" (and ideally\nauto-adjust that N without being told by the user) might be a better\nway to do what you are trying to achieve, but as I already said, I\ncannot read the motivation behind this change that is not explained,\nso...\n"},{"id":"270483","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9D8@mail.accesssoftek.com","threadId":"40395","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9D6@mail.accesssoftek.com","subject":"RE: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-09-22T10:47:45Z","receivedAt":"2015-09-22T10:47:45Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"As for your remark regarding the option naming: \n\n> An option whose name begins with no- looks somewhat strange.  You\ncan even say --no-no-cat-file-batch from the command line, I\nsuspect.\n\nWe already do have some of these: 'no-metadata', 'no-checkout', 'no-auth-cache'. So I was just following the existing convention. Do you think we need to change it and stick with --catch-file-batch=1/--cat-file-batch=0 ?\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Victor Leschuk\nSent: Monday, September 21, 2015 3:03 PM\nTo: Junio C Hamano\nCc: git@vger.kernel.org\nSubject: RE: [PATCH] git-svn: make batch mode optional for git-cat-file\n\nHello Junio,\n\nthanks for your review. First of all I'd like to apologize for sending the patch without description. Actually I was in a hurry and sent it by accident: I planned to edit the mail before sending...\n\nHere is the detailed description:\n\nLast week we had a quick discussion in this mailing list: http://thread.gmane.org/gmane.comp.version-control.git/278021 .\n\nThe thing is that git-cat-file keeps growing during work when running in \"batch\" mode. See the figure attached: it is for cloning a rather small repo (1 hour to clone about ~14000 revisions). However the clone of a large repo (~280000 revisions) took about 2 weeks and git-cat-file has outgrown the parent perl process several times (git-cat-file - ~3-4Gb, perl - 400Mb).\n\nWhat was done:\n * I have run it under valgrind and mtrace and haven't found any memory leaks\n * Found the source of most number of memory reallocations (batch_object_write() function (strbuf_expand -> realloc)) - tried to make the streambuf object static and avoid reallocs - didn't help\n * Tried preloading other allocators than standard glibc - no significant difference\n\nAfter that I replaced the batch mode with separate cat-file calls for each blob and it didn't have any impact on clone performance on real code repositories. However I created a fake test repo with large number of small files (~10 bytes each): here is how I created it https://bitbucket.org/vleschuk/svngenrepo\n\nAnd on this artificial test repo it really slowed down the process. So I decided to suggest to make the batch mode optional to let the user \"tune\" the process and created a patch for this.\n\nAs for your code-style notes, I agree with them, will adjust the code.\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Junio C Hamano [jch2355@gmail.com] On Behalf Of Junio C Hamano [gitster@pobox.com]\nSent: Monday, September 21, 2015 11:25 AM\nTo: Victor Leschuk\nCc: git@vger.kernel.org; Victor Leschuk\nSubject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file\n\nVictor Leschuk <vleschuk@gmail.com> writes:\n\n> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n\nBefore the S-o-b line is a good place to explain why this is a good\nchange to have.  Please use it.\n\n>  git-svn.perl |  1 +\n>  perl/Git.pm  | 41 ++++++++++++++++++++++++++++++++++++++++-\n>  2 files changed, 41 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-svn.perl b/git-svn.perl\n> index 36f7240..b793c26 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' => \\$Git::SVN::_follow_parent,\n>               'use-log-author' => \\$Git::SVN::_use_log_author,\n>               'add-author-from' => \\$Git::SVN::_add_author_from,\n>               'localtime' => \\$Git::SVN::_localtime,\n> +             'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },\n\nAn option whose name begins with no- looks somewhat strange.  You\ncan even say --no-no-cat-file-batch from the command line, I\nsuspect.\n\nWhy not give an option 'cat-file-batch' that sets the variable\n$Git::cat_file_batch to false, and initialize the variable to true\nto keep existing users who do not pass the option happy?\n\n>               %remote_opts );\n>\n>  my ($_trunk, @_tags, @_branches, $_stdlayout);\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 19ef081..69e5293 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);\n>  use Time::Local qw(timegm);\n>  }\n>\n> +our $no_cat_file_batch = 0;\n>\n>  =head1 CONSTRUCTORS\n>\n> @@ -1012,6 +1013,10 @@ returns the number of bytes printed.\n>  =cut\n>\n>  sub cat_blob {\n> +     (1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);\n\nDiscard \"1 ==\" here.  You are clearly using the variable as a\nboolean, so writing this as\n\n        $cat_file_batch ? _cat_blob_batch(@_) : _cat_blob_cmd(@_);\n\nor better yet\n\n        if ($cat_file_batch) {\n                _cat_blob_batch(@_);\n        } else {\n                _cat_blob_cmd(@_);\n        }\n\nwould be more natural.\n\n> +}\n> +\n> +sub _cat_blob_batch {\n>       my ($self, $sha1, $fh) = @_;\n>\n>       $self->_open_cat_blob_if_needed();\n> @@ -1072,7 +1077,7 @@ sub cat_blob {\n>  sub _open_cat_blob_if_needed {\n>       my ($self) = @_;\n>\n> -     return if defined($self->{cat_blob_pid});\n> +     return if ( defined($self->{cat_blob_pid}) || 1 == $no_cat_file_batch );\n\nLikewise.\n\n        return if (!$cat_file_batch);\n        return if defined($self->{cat_blob_pid});\n\n> +sub _cat_blob_cmd {\n> +     my ($self, $sha1, $fh) = @_;\n> +...\n\nThe biggest thing that is missing from this patch is the explanation\nof why this is a good thing to do.  The batch interface was invented\nbecause people found that it was wasteful to spawn a new cat-file\nprocess every time the contents of a blob is needed and wanted to\navoid it, and this new feature gives the user a way to tell Git to\ndo things in a \"wasteful\" way, so there must be a reason why the\nuser would want to use the \"wasteful\" way, perhaps work around some\nother issue.  Without explaining that in the documentation what that\nissue is, i.e. telling users who reads \"git svn --help\" when and why\nthe option might help them, nobody would use the feature to benefit\nfrom it.\n\nI wonder if \"cat-file --batch\" is leaky and bloats after running for\na while.  If that is the case, I have to wonder if \"never do batch\"\nlike this patch does is a sensible way forward.  Instead, \"recycle\nand renew the process after running it for N requests\" (and ideally\nauto-adjust that N without being told by the user) might be a better\nway to do what you are trying to achieve, but as I already said, I\ncannot read the motivation behind this change that is not explained,\nso...\n"},{"id":"270485","messageId":"xmqqlhby5yj3.fsf@gitster.mtv.corp.google.com","threadId":"40395","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9D8@mail.accesssoftek.com","subject":"Re: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T14:17:20Z","receivedAt":"2015-09-22T14:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victor Leschuk <vleschuk@accesssoftek.com> writes:\n\n> We already do have some of these: 'no-metadata', 'no-checkout',\n> no-auth-cache'. So I was just following the existing convention. Do\n> you think we need to change it and stick with\n> --catch-file-batch=1/--cat-file-batch=0 ?\n\nInventing a new --cat-file-batch=[0|1] is not a good idea, and\ncertainly not what I would suggest at all.\n\nMy suggestion was to accept --cat-file-batch to allow the --batch\nprocessing, and to accept--no-cat-file-batch to trigger your new\ncodepath (and leave --cat-file-batch the default when neither is\ngiven).  As these option descriptions are eventually passed to\nGetopt::Long, I thought it should not be too hard to arrange.\n\nMimicking the existing handling of no-whatever is less bad than\naccepting --cat-file-batch=[0|1], if you cannot tell the code to\ntake --[no-]cat-file-batch for whatever reason.  In the longer term\nit would need to be cleaned up together with existing ones.  Your\npatch would be adding another instance that needs to be cleaned up\nto that existing pile, but as long as it follows the same pattern as\nexisting ones, it is easier to spot what needs to be fixed later.\nCompared to that, accepting --cat-file-batch=[0|1] would be far\nworse, as such a future clean-up effort can miss it due to its not\nfollowing the same pattern.\n"},{"id":"270541","messageId":"20150923001350.GA22266@dcvr.yhbt.net","threadId":"40395","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9D6@mail.accesssoftek.com","subject":"Re: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2015-09-23T00:13:50Z","receivedAt":"2015-09-23T00:13:50Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Victor Leschuk <vleschuk@accesssoftek.com> wrote:\n> The thing is that git-cat-file keeps growing during work when running\n> in \"batch\" mode. See the figure attached: it is for cloning a rather\n> small repo (1 hour to clone about ~14000 revisions). However the clone\n> of a large repo (~280000 revisions) took about 2 weeks and\n> git-cat-file has outgrown the parent perl process several times\n> (git-cat-file - ~3-4Gb, perl - 400Mb).\n\nUgh, that sucks.\nEven the 400Mb size of Perl annoys me greatly and I'd work\non fixing it if I had more time.\n\nBut I'm completely against adding this parameter to git-svn.\ngit-svn is not the only \"cat-file --batch\" user, so this option is\nonly hiding problems.\n\nThe best choice is to figure out why cat-file is wasting memory.\n\nDisclaimer: I'm no expert on parts of git written in C,\nbut perhaps the alloc.c interface is why memory keeps growing.\n\n> What was done:\n>  * I have run it under valgrind and mtrace and haven't found any memory leaks\n>  * Found the source of most number of memory reallocations (batch_object_write() function (strbuf_expand -> realloc)) - tried to make the streambuf object static and avoid reallocs - didn't help\n>  * Tried preloading other allocators than standard glibc - no significant difference\n\nA few more questions:\n\n* What is the largest file that existed in that repo?\n\n* Did you try \"MALLOC_MMAP_THRESHOLD_\" with glibc?\n\n  Perhaps setting that to 131072 will help, that'll force releasing\n  larger chunks than that; but it might be moot if alloc.c is\n  getting in the way.\n\nIf alloc.c is the culprit, I would consider to transparently restart\n\"cat-file --batch\" once it grows to a certain size or after a certain\nnumber of requests are made to it.\n\nWe can probably do this inside \"git cat-file\" itself without\nchanging any callers by calling execve.\n"},{"id":"270543","messageId":"20150923003516.GA6086@dcvr.yhbt.net","threadId":"40395","inReplyTo":"20150923001350.GA22266@dcvr.yhbt.net","subject":"Re: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2015-09-23T00:35:16Z","receivedAt":"2015-09-23T00:35:16Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong <normalperson@yhbt.net> wrote:\n> Victor Leschuk <vleschuk@accesssoftek.com> wrote:\n> > The thing is that git-cat-file keeps growing during work when running\n> > in \"batch\" mode. See the figure attached: it is for cloning a rather\n> > small repo (1 hour to clone about ~14000 revisions). However the clone\n> > of a large repo (~280000 revisions) took about 2 weeks and\n> > git-cat-file has outgrown the parent perl process several times\n> > (git-cat-file - ~3-4Gb, perl - 400Mb).\n\nHow much of that is anonymous memory, though?\n(pmap $PID_OF_GIT_CAT_FILE)\n\nRunning the following on the Linux kernel tree I had lying around:\n\n(for i in $(seq 100 200); do git ls-files | sed -e \"s/^/HEAD~$i:/\"; done)|\\\n  git cat-file --batch >/dev/null\n\nReveals about 510M RSS in top, but pmap says less than 20M of that\nis anonymous.  So the rest are mmap-ed packfiles; that RSS gets\ntransparently released back to the kernel under memory pressure.\n"},{"id":"270580","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9DC@mail.accesssoftek.com","threadId":"40395","inReplyTo":"20150923003516.GA6086@dcvr.yhbt.net","subject":"RE: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-09-23T15:28:02Z","receivedAt":"2015-09-23T15:28:02Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello Eric, thanks for looking into it.\n\n>> git-cat-file has outgrown the parent perl process several times\n>> (git-cat-file - ~3-4Gb, perl - 400Mb).\n\n> Ugh, that sucks.\n> Even the 400Mb size of Perl annoys me greatly and I'd work\n> on fixing it if I had more time.\n\nI was going to look at this problem also, but first I'd like to improve the situation with cat-file as on large repos it is larger problem. By the way, what direction would you suggest to begin with?\n\n> A few more questions:\n\n> * What is the largest file that existed in that repo?\n\nAbout 2.5M\n\n> * Did you try \"MALLOC_MMAP_THRESHOLD_\" with glibc?\n\nHave just tried it on a smaller repo (which takes about 1 hour to clone and RSS grows from 4M to 40M during the process. Unfortunately there is no much of an effect: max RSS is 41M with default settings and 38M with MALLOC_MMAP_THRESHOLD_=131072.\n\n> If alloc.c is the culprit, I would consider to transparently restart\n\"cat-file --batch\" once it grows to a certain size or after a certain\nnumber of requests are made to it.\n\nalloc.c interface is not used in cat-file at all, only direct calls to xmalloc and xrealloc from wrapper.c, and also xmmap() from sha1_file.c.\n\n> > git-cat-file has outgrown the parent perl process several times\n> > (git-cat-file - ~3-4Gb, perl - 400Mb).\n\n> How much of that is anonymous memory, though?\n\nHaven't measured on this particular repo: didn't redo the 2 week experiment =) However I checked on a smaller test repo and anon memory is about 12M out of 40M total. Most of memory is really taken by mmaped *.pack and *idx files.\n\nActually I accidentally found out that if I export GIT_MALLOC_LIMIT variable set to several megabytes it has the following effect:\n * git-svn.perl launches git-gc\n * git-gc can't allocate enough memory and thus doesn't create any pack files\n * git-cat-file works only with pure blob object, not packs, and it's memory usage doesn't grow larger than 4-5M\n\nIt gave me a thought that maybe we could get rid of \"git gc\" calls after each commit in perl code and just perform one large gc operation at the end. It will cost disk space during clone but save us memory. What do you think?\n\nAs for your suggestion regarding periodic restart of batch process inside git-cat-file, I think we could give it a try, I can prepare a patch and run some tests.\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Eric Wong [normalperson@yhbt.net]\nSent: Tuesday, September 22, 2015 5:35 PM\nTo: Victor Leschuk\nCc: Junio C Hamano; git@vger.kernel.org\nSubject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file\n\nEric Wong <normalperson@yhbt.net> wrote:\n> Victor Leschuk <vleschuk@accesssoftek.com> wrote:\n> > The thing is that git-cat-file keeps growing during work when running\n> > in \"batch\" mode. See the figure attached: it is for cloning a rather\n> > small repo (1 hour to clone about ~14000 revisions). However the clone\n> > of a large repo (~280000 revisions) took about 2 weeks and\n> > git-cat-file has outgrown the parent perl process several times\n> > (git-cat-file - ~3-4Gb, perl - 400Mb).\n\nHow much of that is anonymous memory, though?\n(pmap $PID_OF_GIT_CAT_FILE)\n\nRunning the following on the Linux kernel tree I had lying around:\n\n(for i in $(seq 100 200); do git ls-files | sed -e \"s/^/HEAD~$i:/\"; done)|\\\n  git cat-file --batch >/dev/null\n\nReveals about 510M RSS in top, but pmap says less than 20M of that\nis anonymous.  So the rest are mmap-ed packfiles; that RSS gets\ntransparently released back to the kernel under memory pressure.\n"},{"id":"270597","messageId":"20150923192212.GA8577@dcvr.yhbt.net","threadId":"40395","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9DC@mail.accesssoftek.com","subject":"Re: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2015-09-23T19:22:12Z","receivedAt":"2015-09-23T19:22:12Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Victor Leschuk <vleschuk@accesssoftek.com> wrote:\n> Hello Eric, thanks for looking into it.\n> \n> >> git-cat-file has outgrown the parent perl process several times\n> >> (git-cat-file - ~3-4Gb, perl - 400Mb).\n> \n> > Ugh, that sucks.\n> > Even the 400Mb size of Perl annoys me greatly and I'd work\n> > on fixing it if I had more time.\n> \n> I was going to look at this problem also, but first I'd like to improve the situation with cat-file as on large repos it is larger problem. By the way, what direction would you suggest to begin with?\n\nSee below :)\n\n<snip anonymous memory stuff, it doesn't seem to be a culprit>\n\n> > > git-cat-file has outgrown the parent perl process several times\n> > > (git-cat-file - ~3-4Gb, perl - 400Mb).\n> \n> > How much of that is anonymous memory, though?\n> \n> Haven't measured on this particular repo: didn't redo the 2 week\n> experiment =) However I checked on a smaller test repo and anon memory\n> is about 12M out of 40M total. Most of memory is really taken by\n> mmaped *.pack and *idx files.\n\nIf it's mmap-ed files, that physical memory is only used on-demand\nand can be dropped at any time because it's backed by disk.\n\nIn other words, I would not worry about any file-backed mmap at all\n(unless you're on 32-bit, but I think git has workarounds for that)\n\nDo you still have that giant repo around?\n\nAre the combined size of the pack + idx files are at least 3-4 GB?\n\nThis should cat all the blobs in history without re-running git-svn:\n\n\tgit log --all --raw -r --no-abbrev | \\\n\t  awk '/^:/ {print $3; print $4}' | git cat-file --batch\n\ngit log actually keeps growing, but the cat-file process shouldn't\nuse anonymous memory much if you inspect it with pmap.\n\n> Actually I accidentally found out that if I export GIT_MALLOC_LIMIT\n> variable set to several megabytes it has the following effect:\n\n>  * git-svn.perl launches git-gc\n>  * git-gc can't allocate enough memory and thus doesn't create any pack files\n>  * git-cat-file works only with pure blob object, not packs, and it's\n> memory usage doesn't grow larger than 4-5M\n> \n> It gave me a thought that maybe we could get rid of \"git gc\" calls\n> after each commit in perl code and just perform one large gc operation\n> at the end. It will cost disk space during clone but save us memory.\n> What do you think?\n\nYou can set gc.auto to zero in your $GIT_CONFIG to disable gc.\nThe \"git gc\" calls were added because unpacked repos were growing\ntoo large and caused problems for other people.\n\nPerhaps play with some other pack* options documented in\nDocumentation/config to limit maximum pack size/depth.\n\nIs this a 32-bit or 64-bit system?\n\n> As for your suggestion regarding periodic restart of batch process\n> inside git-cat-file, I think we could give it a try, I can prepare a\n> patch and run some tests.\n\nI am not sure if we need it for git-svn.\n\nIn another project, the only reason I've found to restart\n\"cat-file --batch\" is in case the repo got repacked and old packs\ngot unlinked, cat-file would hold a reference onto the old file\nand suck up space.   It might be better if \"cat-file --batch\" learned\nto detect unlinked files and then munmap + close them.\n"},{"id":"271409","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9E7@mail.accesssoftek.com","threadId":"40395","inReplyTo":"20150923192212.GA8577@dcvr.yhbt.net","subject":"RE: [PATCH] git-svn: make batch mode optional for git-cat-file","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-10-11T12:31:57Z","receivedAt":"2015-10-11T12:31:57Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello Eric,\n\nThanks for all the advices. I have played with several repositories (both on 32bit and 64bit machines). You were correct most of the memory if used by mapped files and yes it doesn't cause any problems, even a 32bit machine with 500Mb of memory works normally with a heavy loaded git-cat-file.\n\nThanks also for the advice to use git gc config options, I tested gc.auto=0 and it lead to the same behavior as my setting MALLOC_LIMIT, however it is more correct way to get this effect =)\n\nI agree that we shouldn't worry about mapped files.\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Eric Wong [normalperson@yhbt.net]\nSent: Wednesday, September 23, 2015 12:22 PM\nTo: Victor Leschuk\nCc: Junio C Hamano; git@vger.kernel.org\nSubject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file\n\nVictor Leschuk <vleschuk@accesssoftek.com> wrote:\n> Hello Eric, thanks for looking into it.\n>\n> >> git-cat-file has outgrown the parent perl process several times\n> >> (git-cat-file - ~3-4Gb, perl - 400Mb).\n>\n> > Ugh, that sucks.\n> > Even the 400Mb size of Perl annoys me greatly and I'd work\n> > on fixing it if I had more time.\n>\n> I was going to look at this problem also, but first I'd like to improve the situation with cat-file as on large repos it is larger problem. By the way, what direction would you suggest to begin with?\n\nSee below :)\n\n<snip anonymous memory stuff, it doesn't seem to be a culprit>\n\n> > > git-cat-file has outgrown the parent perl process several times\n> > > (git-cat-file - ~3-4Gb, perl - 400Mb).\n>\n> > How much of that is anonymous memory, though?\n>\n> Haven't measured on this particular repo: didn't redo the 2 week\n> experiment =) However I checked on a smaller test repo and anon memory\n> is about 12M out of 40M total. Most of memory is really taken by\n> mmaped *.pack and *idx files.\n\nIf it's mmap-ed files, that physical memory is only used on-demand\nand can be dropped at any time because it's backed by disk.\n\nIn other words, I would not worry about any file-backed mmap at all\n(unless you're on 32-bit, but I think git has workarounds for that)\n\nDo you still have that giant repo around?\n\nAre the combined size of the pack + idx files are at least 3-4 GB?\n\nThis should cat all the blobs in history without re-running git-svn:\n\n        git log --all --raw -r --no-abbrev | \\\n          awk '/^:/ {print $3; print $4}' | git cat-file --batch\n\ngit log actually keeps growing, but the cat-file process shouldn't\nuse anonymous memory much if you inspect it with pmap.\n\n> Actually I accidentally found out that if I export GIT_MALLOC_LIMIT\n> variable set to several megabytes it has the following effect:\n\n>  * git-svn.perl launches git-gc\n>  * git-gc can't allocate enough memory and thus doesn't create any pack files\n>  * git-cat-file works only with pure blob object, not packs, and it's\n> memory usage doesn't grow larger than 4-5M\n>\n> It gave me a thought that maybe we could get rid of \"git gc\" calls\n> after each commit in perl code and just perform one large gc operation\n> at the end. It will cost disk space during clone but save us memory.\n> What do you think?\n\nYou can set gc.auto to zero in your $GIT_CONFIG to disable gc.\nThe \"git gc\" calls were added because unpacked repos were growing\ntoo large and caused problems for other people.\n\nPerhaps play with some other pack* options documented in\nDocumentation/config to limit maximum pack size/depth.\n\nIs this a 32-bit or 64-bit system?\n\n> As for your suggestion regarding periodic restart of batch process\n> inside git-cat-file, I think we could give it a try, I can prepare a\n> patch and run some tests.\n\nI am not sure if we need it for git-svn.\n\nIn another project, the only reason I've found to restart\n\"cat-file --batch\" is in case the repo got repacked and old packs\ngot unlinked, cat-file would hold a reference onto the old file\nand suck up space.   It might be better if \"cat-file --batch\" learned\nto detect unlinked files and then munmap + close them.\n"}]}