diff mbox series

[v3,03/27] refs/reftable: stop micro-optimizing refname allocations on copy

Message ID 085d90c8b63e937e3d76637ce2422320b5801ebb.1717402403.git.ps@pks.im (mailing list archive)
State Accepted
Commit 23c32511b31f2123fd194ba3a89c758ba3e55143
Headers show
Series [v3,01/27] global: improve const correctness when assigning string constants | expand

Commit Message

Patrick Steinhardt June 3, 2024, 9:39 a.m. UTC
When copying refs, we execute `write_copy_table()` to write the new
table. As the names arge given to use via `arg->newname` and
`arg->oldname`, respectively, we optimize away some allocations by
assigning those fields to the reftable records we are about to write.
This requires us to cast the input to `char *` pointers as they are in
fact constant strings. Later on, we then unset the refname for all of
the records before calling `reftable_log_record_release()` on them.

We also do this when assigning the "HEAD" constant, but here we do not
cast because its type is `char[]` by default. It's about to be turned
into `const char *` though once we enable `-Wwrite-strings` and will
thus cause another warning.

It's quite dubious whether this micro-optimization really helps. We're
about to write to disk anyway, which is going to be way slower than a
small handful of allocations. Let's drop the optimization altogther and
instead copy arguments to simplify the code and avoid the future warning
with `-Wwrite-strings`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

Comments

Junio C Hamano June 3, 2024, 6:08 p.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> When copying refs, we execute `write_copy_table()` to write the new
> table. As the names arge given to use via `arg->newname` and

arge???

> `arg->oldname`, respectively, we optimize away some allocations by
> assigning those fields to the reftable records we are about to write.
> This requires us to cast the input to `char *` pointers as they are in
> fact constant strings. Later on, we then unset the refname for all of
> the records before calling `reftable_log_record_release()` on them.
>
> We also do this when assigning the "HEAD" constant, but here we do not
> cast because its type is `char[]` by default. It's about to be turned
> into `const char *` though once we enable `-Wwrite-strings` and will
> thus cause another warning.
>
> It's quite dubious whether this micro-optimization really helps. We're
> about to write to disk anyway, which is going to be way slower than a
> small handful of allocations. Let's drop the optimization altogther and
> instead copy arguments to simplify the code and avoid the future warning
> with `-Wwrite-strings`.

It certainly makes the final clean-up part simpler, which is a good
sign.
diff mbox series

Patch

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 1af86bbdec..e77faa2b9d 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1340,10 +1340,10 @@  static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 	 * old reference.
 	 */
 	refs[0] = old_ref;
-	refs[0].refname = (char *)arg->newname;
+	refs[0].refname = xstrdup(arg->newname);
 	refs[0].update_index = creation_ts;
 	if (arg->delete_old) {
-		refs[1].refname = (char *)arg->oldname;
+		refs[1].refname = xstrdup(arg->oldname);
 		refs[1].value_type = REFTABLE_REF_DELETION;
 		refs[1].update_index = deletion_ts;
 	}
@@ -1366,7 +1366,7 @@  static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 		ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
 		memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
 		fill_reftable_log_record(&logs[logs_nr], &committer_ident);
-		logs[logs_nr].refname = (char *)arg->newname;
+		logs[logs_nr].refname = xstrdup(arg->newname);
 		logs[logs_nr].update_index = deletion_ts;
 		logs[logs_nr].value.update.message =
 			xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
@@ -1387,7 +1387,13 @@  static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 		if (append_head_reflog) {
 			ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
 			logs[logs_nr] = logs[logs_nr - 1];
-			logs[logs_nr].refname = "HEAD";
+			logs[logs_nr].refname = xstrdup("HEAD");
+			logs[logs_nr].value.update.name =
+				xstrdup(logs[logs_nr].value.update.name);
+			logs[logs_nr].value.update.email =
+				xstrdup(logs[logs_nr].value.update.email);
+			logs[logs_nr].value.update.message =
+				xstrdup(logs[logs_nr].value.update.message);
 			logs_nr++;
 		}
 	}
@@ -1398,7 +1404,7 @@  static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 	ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
 	memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
 	fill_reftable_log_record(&logs[logs_nr], &committer_ident);
-	logs[logs_nr].refname = (char *)arg->newname;
+	logs[logs_nr].refname = xstrdup(arg->newname);
 	logs[logs_nr].update_index = creation_ts;
 	logs[logs_nr].value.update.message =
 		xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
@@ -1430,7 +1436,7 @@  static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 		 */
 		ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
 		logs[logs_nr] = old_log;
-		logs[logs_nr].refname = (char *)arg->newname;
+		logs[logs_nr].refname = xstrdup(arg->newname);
 		logs_nr++;
 
 		/*
@@ -1439,7 +1445,7 @@  static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 		if (arg->delete_old) {
 			ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
 			memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
-			logs[logs_nr].refname = (char *)arg->oldname;
+			logs[logs_nr].refname = xstrdup(arg->oldname);
 			logs[logs_nr].value_type = REFTABLE_LOG_DELETION;
 			logs[logs_nr].update_index = old_log.update_index;
 			logs_nr++;
@@ -1462,13 +1468,11 @@  static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 	reftable_iterator_destroy(&it);
 	string_list_clear(&skip, 0);
 	strbuf_release(&errbuf);
-	for (i = 0; i < logs_nr; i++) {
-		if (!strcmp(logs[i].refname, "HEAD"))
-			continue;
-		logs[i].refname = NULL;
+	for (i = 0; i < logs_nr; i++)
 		reftable_log_record_release(&logs[i]);
-	}
 	free(logs);
+	for (i = 0; i < ARRAY_SIZE(refs); i++)
+		reftable_ref_record_release(&refs[i]);
 	reftable_ref_record_release(&old_ref);
 	reftable_log_record_release(&old_log);
 	return ret;