git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] convert: avoid malloc of original file size
@ 2019-03-07 19:56 Jeff King
  2019-03-08  1:26 ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2019-03-07 19:56 UTC (permalink / raw)
  To: git; +Cc: Joey Hess, Junio C Hamano

From: Joey Hess <id@joeyh.name>

We write the output of a "clean" filter into a strbuf. Rather than
growing the strbuf dynamically as we read its output, we make the
initial allocation as large as the original input file. This is a good
guess when the filter is just tweaking a few bytes, but it's disastrous
when the point of the filter is to condense a very large file into a
short identifier (e.g., the way git-lfs and git-annex do). We may ask to
allocate many gigabytes, causing the allocation to fail and Git to
die().

Instead, let's just let strbuf do its usual growth.

When the clean filter does output something around the same size as the
worktree file, the buffer will need to be reallocated until it fits,
starting at 8192 and doubling in size. Benchmarking indicates that
reallocation is not a significant overhead for outputs up to a
few MB in size.

Signed-off-by: Joey Hess <id@joeyh.name>
Signed-off-by: Jeff King <peff@peff.net>
---
This is a resurrection of the patch from:

  https://public-inbox.org/git/20190122220714.GA6176@kitenet.net/

It got stalled on discussion of the commit message, which I've rewritten
here to match the suggestions in the thread.

As discussed there, I do think this only solves half the problem, as the
smudge filter has the same issue in reverse. That's more complicated to
fix, and AFAIK nobody is working on it. But I don't think there's any
reason not to pick up this part in the meantime.

 convert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index 5d0307fc10..94ff837649 100644
--- a/convert.c
+++ b/convert.c
@@ -731,7 +731,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le
 	if (start_async(&async))
 		return 0;	/* error was already reported */
 
-	if (strbuf_read(&nbuf, async.out, len) < 0) {
+	if (strbuf_read(&nbuf, async.out, 0) < 0) {
 		err = error(_("read from external filter '%s' failed"), cmd);
 	}
 	if (close(async.out)) {
-- 
2.21.0.787.g929e938557

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] convert: avoid malloc of original file size
  2019-03-07 19:56 [PATCH] convert: avoid malloc of original file size Jeff King
@ 2019-03-08  1:26 ` Junio C Hamano
  2019-03-08  2:52   ` Jeff King
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2019-03-08  1:26 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Joey Hess

Jeff King <peff@peff.net> writes:

> As discussed there, I do think this only solves half the problem, as the
> smudge filter has the same issue in reverse. That's more complicated to
> fix, and AFAIK nobody is working on it. But I don't think there's any
> reason not to pick up this part in the meantime.

Yeah, I agree that the reverse direction shares the same issue.

I am not sure 0 is a good initial value in this direction, either;
I'd rather clip to min(len, core.bigfilethreshold) or something like
that, to avoid regressing the more normal use cases.  

But let's queue this and see what happens.

Thanks.

>
>  convert.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/convert.c b/convert.c
> index 5d0307fc10..94ff837649 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -731,7 +731,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le
>  	if (start_async(&async))
>  		return 0;	/* error was already reported */
>  
> -	if (strbuf_read(&nbuf, async.out, len) < 0) {
> +	if (strbuf_read(&nbuf, async.out, 0) < 0) {
>  		err = error(_("read from external filter '%s' failed"), cmd);
>  	}
>  	if (close(async.out)) {

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] convert: avoid malloc of original file size
  2019-03-08  1:26 ` Junio C Hamano
@ 2019-03-08  2:52   ` Jeff King
  0 siblings, 0 replies; 3+ messages in thread
From: Jeff King @ 2019-03-08  2:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Joey Hess

On Fri, Mar 08, 2019 at 10:26:24AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > As discussed there, I do think this only solves half the problem, as the
> > smudge filter has the same issue in reverse. That's more complicated to
> > fix, and AFAIK nobody is working on it. But I don't think there's any
> > reason not to pick up this part in the meantime.
> 
> Yeah, I agree that the reverse direction shares the same issue.
> 
> I am not sure 0 is a good initial value in this direction, either;
> I'd rather clip to min(len, core.bigfilethreshold) or something like
> that, to avoid regressing the more normal use cases.

That was my initial thought, too, but Joey's benchmarks show that it
doesn't seem to make a big difference either way. In his numbers it did
get measurable for a 1GB file, but we'd still not use "hint == len" in
that case (we'd probably do one or two doublings to get there).

I also think running a real (non-condensing) filter on a 1GB file is
already a pretty unlikely corner case.

> But let's queue this and see what happens.

Sounds good to me.

-Peff

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-03-08  2:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07 19:56 [PATCH] convert: avoid malloc of original file size Jeff King
2019-03-08  1:26 ` Junio C Hamano
2019-03-08  2:52   ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git