@matheusbsbs / matheus-bagliano / commits / 23c32511b3

refs/reftable: stop micro-optimizing refname allocations on copy

When copying refs, we execute `write_copy_table()` to write the new table. As the names are given to us 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 directly, without duplicating them. 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Jun 7, 2024 at 08:37 UTC 23c32511b31f2123fd194ba3a89c758ba3e55143

1 file changed +16 -12

refs/reftable-backend.c

+16 -12

@@ -1340,10 +1340,10 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)

1340 * old reference.

1341 */

1342 refs[0] = old_ref;

1343 - refs[0].refname = (char *)arg->newname;

1343 + refs[0].refname = xstrdup(arg->newname);

1344 refs[0].update_index = creation_ts;

1345 if (arg->delete_old) {

1346 - refs[1].refname = (char *)arg->oldname;

1346 + refs[1].refname = xstrdup(arg->oldname);

1347 refs[1].value_type = REFTABLE_REF_DELETION;

1348 refs[1].update_index = deletion_ts;

1349 }

@@ -1366,7 +1366,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)

1366 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);

1367 memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));

1368 fill_reftable_log_record(&logs[logs_nr], &committer_ident);

1369 - logs[logs_nr].refname = (char *)arg->newname;

1369 + logs[logs_nr].refname = xstrdup(arg->newname);

1370 logs[logs_nr].update_index = deletion_ts;

1371 logs[logs_nr].value.update.message =

1372 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)

1387 if (append_head_reflog) {

1388 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);

1389 logs[logs_nr] = logs[logs_nr - 1];

1390 - logs[logs_nr].refname = "HEAD";

1390 + logs[logs_nr].refname = xstrdup("HEAD");

1391 + logs[logs_nr].value.update.name =

1392 + xstrdup(logs[logs_nr].value.update.name);

1393 + logs[logs_nr].value.update.email =

1394 + xstrdup(logs[logs_nr].value.update.email);

1395 + logs[logs_nr].value.update.message =

1396 + xstrdup(logs[logs_nr].value.update.message);

1397 logs_nr++;

1398 }

1399 }

@@ -1398,7 +1404,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)

1404 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);

1405 memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));

1406 fill_reftable_log_record(&logs[logs_nr], &committer_ident);

1401 - logs[logs_nr].refname = (char *)arg->newname;

1407 + logs[logs_nr].refname = xstrdup(arg->newname);

1408 logs[logs_nr].update_index = creation_ts;

1409 logs[logs_nr].value.update.message =

1410 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)

1436 */

1437 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);

1438 logs[logs_nr] = old_log;

1433 - logs[logs_nr].refname = (char *)arg->newname;

1439 + logs[logs_nr].refname = xstrdup(arg->newname);

1440 logs_nr++;

1441

1442 /*

@@ -1439,7 +1445,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)

1445 if (arg->delete_old) {

1446 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);

1447 memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));

1442 - logs[logs_nr].refname = (char *)arg->oldname;

1448 + logs[logs_nr].refname = xstrdup(arg->oldname);

1449 logs[logs_nr].value_type = REFTABLE_LOG_DELETION;

1450 logs[logs_nr].update_index = old_log.update_index;

1451 logs_nr++;

@@ -1462,13 +1468,11 @@ done:

1468 reftable_iterator_destroy(&it);

1469 string_list_clear(&skip, 0);

1470 strbuf_release(&errbuf);

1465 - for (i = 0; i < logs_nr; i++) {

1466 - if (!strcmp(logs[i].refname, "HEAD"))

1467 - continue;

1468 - logs[i].refname = NULL;

1471 + for (i = 0; i < logs_nr; i++)

1472 reftable_log_record_release(&logs[i]);

1470 - }

1473 free(logs);

1474 + for (i = 0; i < ARRAY_SIZE(refs); i++)

1475 + reftable_ref_record_release(&refs[i]);

1476 reftable_ref_record_release(&old_ref);

1477 reftable_log_record_release(&old_log);

1478 return ret;