[PATCH 0/2] refs: stylistic improvements to ref migration optimization

[PATCH 0/2] refs: stylistic improvements to ref migration optimization

[Patrick Steinhardt]

Hi,

this small patch series addresses some stylistic nits surfaced in the
review of ps/ref-backend-migration-optim. v3 of that series and the
merge to `next` have crossed, so I'm resending the changes from v3 on
top of what we have in `next`.

This version is built on top of 6ea2d9d271 (Sync with Git 2.47.1,
2024-11-25) with ps/ref-backend-migration-optim at d94ac23d3b
(reftable/block: optimize allocations by using scratch buffer,
2024-11-20) merged into it.

Thanks!

Patrick

---
Patrick Steinhardt (2):
      refs: adapt `initial_transaction` flag to be unsigned
      reftable: rename scratch buffer

 refs.c            |  2 +-
 refs.h            |  2 +-
 reftable/block.c  | 10 +++++-----
 reftable/block.h  |  3 ++-
 reftable/writer.c | 22 +++++++++++-----------
 reftable/writer.h |  3 ++-
 6 files changed, 22 insertions(+), 20 deletions(-)


---
base-commit: a468246cedfb3b54b03609be75d9e13f0be1cc29
change-id: 20241125-pks-refs-migration-optimization-followup-d766e86b8925

[PATCH 1/2] refs: adapt `initial_transaction` flag to be unsigned

[Patrick Steinhardt]

The `initial_transaction` flag is tracked as a signed integer, but we
typically pass around flags via unsigned integers. Adapt the type
accordingly.

Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 2 +-
 refs.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 65eea3eb7734d03f09a22e8edfe5074d532398ff..ee870817466b7d6d6a6619ce0baffe17f3d5a39f 100644
--- a/refs.c
+++ b/refs.c
@@ -2325,7 +2325,7 @@ int refs_verify_refname_available(struct ref_store *refs,
 				  const char *refname,
 				  const struct string_list *extras,
 				  const struct string_list *skip,
-				  int initial_transaction,
+				  unsigned int initial_transaction,
 				  struct strbuf *err)
 {
 	const char *slash;
diff --git a/refs.h b/refs.h
index 980bd20cf24e15144aeff991eeba8b27a936d386..95baf194ba9493f4e8f1f70924f0eb713e5bbd49 100644
--- a/refs.h
+++ b/refs.h
@@ -110,7 +110,7 @@ int refs_verify_refname_available(struct ref_store *refs,
 				  const char *refname,
 				  const struct string_list *extras,
 				  const struct string_list *skip,
-				  int initial_transaction,
+				  unsigned int initial_transaction,
 				  struct strbuf *err);
 
 int refs_ref_exists(struct ref_store *refs, const char *refname);

-- 
2.47.0.274.g962d0b743d.dirty

[PATCH 2/2] reftable: rename scratch buffer

[Patrick Steinhardt]

Both `struct block_writer` and `struct reftable_writer` have a `buf`
member that is being reused to optimize the number of allocations.
Rename the variable to `scratch` to clarify its intend and provide a
comment explaining why it exists.

Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/block.c  | 10 +++++-----
 reftable/block.h  |  3 ++-
 reftable/writer.c | 22 +++++++++++-----------
 reftable/writer.h |  3 ++-
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/reftable/block.c b/reftable/block.c
index 1aa7e8cd3cbf0980f6bc20262be89e755d0a4b4b..01980784854cc454938bd2278b94047ff62c20d4 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -115,16 +115,16 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	int n = 0;
 	int err;
 
-	err = reftable_record_key(rec, &w->buf);
+	err = reftable_record_key(rec, &w->scratch);
 	if (err < 0)
 		goto done;
 
-	if (!w->buf.len) {
+	if (!w->scratch.len) {
 		err = REFTABLE_API_ERROR;
 		goto done;
 	}
 
-	n = reftable_encode_key(&is_restart, out, last, w->buf,
+	n = reftable_encode_key(&is_restart, out, last, w->scratch,
 				reftable_record_val_type(rec));
 	if (n < 0) {
 		err = -1;
@@ -140,7 +140,7 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	string_view_consume(&out, n);
 
 	err = block_writer_register_restart(w, start.len - out.len, is_restart,
-					    &w->buf);
+					    &w->scratch);
 done:
 	return err;
 }
@@ -565,7 +565,7 @@ void block_writer_release(struct block_writer *bw)
 	REFTABLE_FREE_AND_NULL(bw->zstream);
 	REFTABLE_FREE_AND_NULL(bw->restarts);
 	REFTABLE_FREE_AND_NULL(bw->compressed);
-	reftable_buf_release(&bw->buf);
+	reftable_buf_release(&bw->scratch);
 	reftable_buf_release(&bw->last_key);
 	/* the block is not owned. */
 }
diff --git a/reftable/block.h b/reftable/block.h
index d76f00553073c10185e716e71e2f415ce5dcf7e2..0431e8591f41dedfb96eef304ea63ef2e9e5f5dd 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -39,7 +39,8 @@ struct block_writer {
 	uint32_t restart_cap;
 
 	struct reftable_buf last_key;
-	struct reftable_buf buf;
+	/* Scratch buffer used to avoid allocations. */
+	struct reftable_buf scratch;
 	int entries;
 };
 
diff --git a/reftable/writer.c b/reftable/writer.c
index 6501376ce826469556a7dfa3c39258847300ae66..be0fae6cb229411258d40b8865c2fdee88fd5bcd 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -148,7 +148,7 @@ int reftable_writer_new(struct reftable_writer **out,
 
 	reftable_buf_init(&wp->block_writer_data.last_key);
 	reftable_buf_init(&wp->last_key);
-	reftable_buf_init(&wp->buf);
+	reftable_buf_init(&wp->scratch);
 	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
 	if (!wp->block) {
 		reftable_free(wp);
@@ -181,7 +181,7 @@ static void writer_release(struct reftable_writer *w)
 		w->block_writer = NULL;
 		writer_clear_index(w);
 		reftable_buf_release(&w->last_key);
-		reftable_buf_release(&w->buf);
+		reftable_buf_release(&w->scratch);
 	}
 }
 
@@ -253,17 +253,17 @@ static int writer_add_record(struct reftable_writer *w,
 {
 	int err;
 
-	err = reftable_record_key(rec, &w->buf);
+	err = reftable_record_key(rec, &w->scratch);
 	if (err < 0)
 		goto done;
 
-	if (reftable_buf_cmp(&w->last_key, &w->buf) >= 0) {
+	if (reftable_buf_cmp(&w->last_key, &w->scratch) >= 0) {
 		err = REFTABLE_API_ERROR;
 		goto done;
 	}
 
 	reftable_buf_reset(&w->last_key);
-	err = reftable_buf_add(&w->last_key, w->buf.buf, w->buf.len);
+	err = reftable_buf_add(&w->last_key, w->scratch.buf, w->scratch.len);
 	if (err < 0)
 		goto done;
 
@@ -339,25 +339,25 @@ int reftable_writer_add_ref(struct reftable_writer *w,
 		goto out;
 
 	if (!w->opts.skip_index_objects && reftable_ref_record_val1(ref)) {
-		reftable_buf_reset(&w->buf);
-		err = reftable_buf_add(&w->buf, (char *)reftable_ref_record_val1(ref),
+		reftable_buf_reset(&w->scratch);
+		err = reftable_buf_add(&w->scratch, (char *)reftable_ref_record_val1(ref),
 				       hash_size(w->opts.hash_id));
 		if (err < 0)
 			goto out;
 
-		err = writer_index_hash(w, &w->buf);
+		err = writer_index_hash(w, &w->scratch);
 		if (err < 0)
 			goto out;
 	}
 
 	if (!w->opts.skip_index_objects && reftable_ref_record_val2(ref)) {
-		reftable_buf_reset(&w->buf);
-		err = reftable_buf_add(&w->buf, reftable_ref_record_val2(ref),
+		reftable_buf_reset(&w->scratch);
+		err = reftable_buf_add(&w->scratch, reftable_ref_record_val2(ref),
 				       hash_size(w->opts.hash_id));
 		if (err < 0)
 			goto out;
 
-		err = writer_index_hash(w, &w->buf);
+		err = writer_index_hash(w, &w->scratch);
 		if (err < 0)
 			goto out;
 	}
diff --git a/reftable/writer.h b/reftable/writer.h
index 421a897dccd85ad0532860ff1b4f38b2813d438d..1f4788a430c52c5387b5e97f639e84544d0b9ba2 100644
--- a/reftable/writer.h
+++ b/reftable/writer.h
@@ -20,7 +20,8 @@ struct reftable_writer {
 	void *write_arg;
 	int pending_padding;
 	struct reftable_buf last_key;
-	struct reftable_buf buf;
+	/* Scratch buffer used to avoid allocations. */
+	struct reftable_buf scratch;
 
 	/* offset of next block to write. */
 	uint64_t next;

-- 
2.47.0.274.g962d0b743d.dirty

Re: [PATCH 0/2] refs: stylistic improvements to ref migration optimization

[Christian Couder]

Hi,

On Mon, Nov 25, 2024 at 8:35 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Hi,
>
> this small patch series addresses some stylistic nits surfaced in the
> review of ps/ref-backend-migration-optim. v3 of that series and the
> merge to `next` have crossed, so I'm resending the changes from v3 on
> top of what we have in `next`.

I took a look and both patches in this series look good and correct to me. Ack!

Re: [PATCH 0/2] refs: stylistic improvements to ref migration optimization

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Mon, Nov 25, 2024 at 8:35 AM Patrick Steinhardt <ps@pks.im> wrote:
>>
>> Hi,
>>
>> this small patch series addresses some stylistic nits surfaced in the
>> review of ps/ref-backend-migration-optim. v3 of that series and the
>> merge to `next` have crossed, so I'm resending the changes from v3 on
>> top of what we have in `next`.
>
> I took a look and both patches in this series look good and correct to me. Ack!

Looking good.  Thanks, both of you.

Re: [PATCH 1/2] refs: adapt `initial_transaction` flag to be unsigned

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The `initial_transaction` flag is tracked as a signed integer, but we
> typically pass around flags via unsigned integers. Adapt the type
> accordingly.

It would not matter while the information in the variable is 1 bit
and the check is "is the whole variable 0?", which is what the
current code does on this parameter.  But when we'd start using the
parameter as a flag word to stuff more bits, this starts to matter.

Good future-proofing.  Thanks.