Re: [PATCH 3/6] fast-import: release unfreed strbufs

From: Patrick Steinhardt <ps@pks.im>
To: Thalia Archibald <thalia@archibald.dev>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 3/6] fast-import: release unfreed strbufs
Date: Thu, 28 Mar 2024 09:21:11 +0100	[thread overview]
Message-ID: <ZgUod3QKptINaPBF@tanuki> (raw)
In-Reply-To: <20240322000304.76810-4-thalia@archibald.dev>

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

On Fri, Mar 22, 2024 at 12:03:33AM +0000, Thalia Archibald wrote:
> These strbufs are owned. Release them at the end of their scopes.
> 
> Signed-off-by: Thalia Archibald <thalia@archibald.dev>
> ---
>  builtin/fast-import.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 1b3d6784c1..d6f998f363 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -2364,6 +2364,7 @@ static void file_change_m(const char *p, struct branch *b)
>  	/* Git does not track empty, non-toplevel directories. */
>  	if (S_ISDIR(mode) && is_empty_tree_oid(&oid) && *path.buf) {
>  		tree_content_remove(&b->branch_tree, path.buf, NULL, 0);
> +		strbuf_release(&path);
>  		return;
>  	}

Oh, now you get to my comment in the preceding patch. With this patch
we're now in a somewhat weird in-between state where the buffers are
still static, but we release their memory after each call. So we kind of
get the worst of both worlds: static variables without being able to
reuse the buffer's memory.

If we were to change this then we should definitely mark the buffers as
non-static. If so, it would be great to demonstrate that this does not
significantly impact performance.

The same is true for all the other instances.

Patrick

> @@ -2409,11 +2410,11 @@ static void file_change_m(const char *p, struct branch *b)
>  				command_buf.buf);
>  	}
>  
> -	if (!*path.buf) {
> +	if (*path.buf)
> +		tree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL);
> +	else
>  		tree_content_replace(&b->branch_tree, &oid, mode, NULL);
> -		return;
> -	}
> -	tree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL);
> +	strbuf_release(&path);
>  }
>  
>  static void file_change_d(const char *p, struct branch *b)
> @@ -2422,6 +2423,7 @@ static void file_change_d(const char *p, struct branch *b)
>  
>  	parse_path_eol(&path, p, "path");
>  	tree_content_remove(&b->branch_tree, path.buf, NULL, 1);
> +	strbuf_release(&path);
>  }
>  
>  static void file_change_cr(const char *p, struct branch *b, int rename)
> @@ -2440,17 +2442,18 @@ static void file_change_cr(const char *p, struct branch *b, int rename)
>  		tree_content_get(&b->branch_tree, source.buf, &leaf, 1);
>  	if (!leaf.versions[1].mode)
>  		die("Path %s not in branch", source.buf);
> -	if (!*dest.buf) {	/* C "path/to/subdir" "" */
> +	if (*dest.buf)
> +		tree_content_set(&b->branch_tree, dest.buf,
> +			&leaf.versions[1].oid,
> +			leaf.versions[1].mode,
> +			leaf.tree);
> +	else	/* C "path/to/subdir" "" */
>  		tree_content_replace(&b->branch_tree,
>  			&leaf.versions[1].oid,
>  			leaf.versions[1].mode,
>  			leaf.tree);
> -		return;
> -	}
> -	tree_content_set(&b->branch_tree, dest.buf,
> -		&leaf.versions[1].oid,
> -		leaf.versions[1].mode,
> -		leaf.tree);
> +	strbuf_release(&source);
> +	strbuf_release(&dest);
>  }
>  
>  static void note_change_n(const char *p, struct branch *b, unsigned char *old_fanout)
> @@ -2804,6 +2807,7 @@ static void parse_new_commit(const char *arg)
>  	free(author);
>  	free(committer);
>  	free(encoding);
> +	strbuf_release(&msg);
>  
>  	if (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))
>  		b->pack_id = pack_id;
> @@ -2886,6 +2890,7 @@ static void parse_new_tag(const char *arg)
>  	strbuf_addch(&new_data, '\n');
>  	strbuf_addbuf(&new_data, &msg);
>  	free(tagger);
> +	strbuf_release(&msg);
>  
>  	if (store_object(OBJ_TAG, &new_data, NULL, &t->oid, next_mark))
>  		t->pack_id = MAX_PACK_ID;
> @@ -3171,6 +3176,7 @@ static void print_ls(int mode, const unsigned char *hash, const char *path)
>  		strbuf_addch(&line, '\n');
>  	}
>  	cat_blob_write(line.buf, line.len);
> +	strbuf_release(&line);
>  }
>  
>  static void parse_ls(const char *p, struct branch *b)
> @@ -3206,6 +3212,7 @@ static void parse_ls(const char *p, struct branch *b)
>  		release_tree_content_recursive(leaf.tree);
>  	if (!b || root != &b->branch_tree)
>  		release_tree_entry(root);
> +	strbuf_release(&path);
>  }
>  
>  static void checkpoint(void)
> -- 
> 2.44.0
> 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]