Re: [PATCH 10/15] refs.c: allow multiple reflog updates during a single transaction
On Thu, Oct 23, 2014 at 11:54 AM, Junio C Hamano wrote:
> Ronnie Sahlberg writes:
>
>> @@ -3531,7 +3537,7 @@ struct ref_update {
>> enum transaction_update_type update_type;
>> unsigned char new_sha1[20];
>> unsigned char old_sha1[20];
>> - int flags; /* REF_NODEREF? */
>> + int flags; /* REF_NODEREF? or private flags */
>
> Not a very informative comment, I'd have to say. How are users of
> this API expected to avoid stepping on each others' and API
> implementation's toes?
That is an old and bitrotted comment.
I changed it to point to the canonical definition of these flags instead :
int flags; /* The flags to transaction_update_ref[log] are defined
* in refs.h
*/
>
>> @@ -3539,8 +3545,9 @@ struct ref_update {
>>
>> /* used by reflog updates */
>> int reflog_fd;
>> - struct lock_file reflog_lock;
>> + struct lock_file *reflog_lock;
>
> What is this change about?
>
We have one update entry for each line we want to write to the reflog.
So for the first update to a reflog we allocate a lock_file structure.
For any subsequent reflog entry to the same ref we just let the
pointer point to the previous structure we already allocated.
I.e. a way to have only one lock_file structure and share it across
multiple struct ref_update structures.
> Does the lifetime rule for "struct lock_file" described in
> Documentation/technical/api-lockfile.txt, namely, "once you call
> hold_lock_file_* family on it, you cannot free it yourself", have
> any implication on this?
Nope.
>
>> + if (!(update->flags & UPDATE_REFLOG_NOLOCK))
>> + update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
>> +
>
> Hmph, does this mean that the caller needs to keep track of the refs
> it ever touched inside a single transaction, call this without nolock
> on the first invocation on a particular ref and with nolock on the
> subsequent invocation?
Nope. This is not visible to the caller and is managed transparently
inside the transaction code.
>
> Or is the "caller" just implementation detail of the API and higher level
> callers do not have to care?
The latter,the higher level do not have to care.
regards
ronnie sahlberg
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/15] refs.c: allow multiple reflog updates during a single transaction
Ronnie Sahlberg writes:
> @@ -3531,7 +3537,7 @@ struct ref_update {
> enum transaction_update_type update_type;
> unsigned char new_sha1[20];
> unsigned char old_sha1[20];
> - int flags; /* REF_NODEREF? */
> + int flags; /* REF_NODEREF? or private flags */
Not a very informative comment, I'd have to say. How are users of
this API expected to avoid stepping on each others' and API
implementation's toes?
> @@ -3539,8 +3545,9 @@ struct ref_update {
>
> /* used by reflog updates */
> int reflog_fd;
> - struct lock_file reflog_lock;
> + struct lock_file *reflog_lock;
What is this change about?
Does the lifetime rule for "struct lock_file" described in
Documentation/technical/api-lockfile.txt, namely, "once you call
hold_lock_file_* family on it, you cannot free it yourself", have
any implication on this?
> + if (!(update->flags & UPDATE_REFLOG_NOLOCK))
> + update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
> +
Hmph, does this mean that the caller needs to keep track of the refs
it ever touched inside a single transaction, call this without nolock
on the first invocation on a particular ref and with nolock on the
subsequent invocation?
Or is the "caller" just implementation detail of the API and higher level
callers do not have to care?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/15] refs.c: allow multiple reflog updates during a single transaction
commit 5fd1fa7d8520cf03e94fa7d0e9aa8685de0ef63f upstream.
Allow to make multiple reflog updates to the same ref during a transaction.
This means we only need to lock the reflog once, during the first update
that touches the reflog, and that all further updates can just write the
reflog entry since the reflog is already locked.
This allows us to write code such as:
t = transaction_begin()
transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
loop-over-something...
transaction_reflog_update(t, "foo", 0, );
transaction_commit(t)
where we first truncate the reflog and then build the new content one line
at a time.
While this technically looks like O(n2) behavior it not that bad.
We only do this loop for transactions that cover a single ref during
reflog expire. This means that the linear search inside
transaction_update_reflog() will find the match on the very first entry
thus making it O(1) and not O(n) or our usecases. Thus the whole expire
becomes O(n) instead of O(n2). If in the future we start doing this for many
refs in one single transaction we might want to optimize this.
But there is no need to complexify the code and optimize for future usecases
that might never materialize at this stage.
Change-Id: Ibe703310e33a4db07e5bcc704310211268b788fc
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
refs.c | 46 +-
1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index f14b76e..f7e947f 100644
--- a/refs.c
+++ b/refs.c
@@ -31,6 +31,12 @@ static unsigned char refname_disposition[256] = {
*/
#define REF_ISPRUNING 0x0100
/*
+ * Only the first reflog update needs to lock the reflog file. Further updates
+ * just use the lock taken by the first update.
+ */
+#define UPDATE_REFLOG_NOLOCK 0x0200
+
+/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
* not legal. It is legal if it is something reasonable to have under
@@ -3521,7 +3527,7 @@ enum transaction_update_type {
UPDATE_LOG = 1
};
-/**
+/*
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
* while locking the ref, set have_old to 1 and set old_sha1 to the
@@ -3531,7 +3537,7 @@ struct ref_update {
enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
- int flags; /* REF_NODEREF? */
+ int flags; /* REF_NODEREF? or private flags */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
int type;
@@ -3539,8 +3545,9 @@ struct ref_update {
/* used by reflog updates */
int reflog_fd;
- struct lock_file reflog_lock;
+ struct lock_file *reflog_lock;
char *committer;
+ struct ref_update *orig_update; /* For UPDATE_REFLOG_NOLOCK */
const char refname[FLEX_ARRAY];
};
@@ -3619,11 +3626,26 @@ int transaction_update_reflog(struct transaction
*transaction,
struct strbuf *err)
{
struct ref_update *update;
+ int i;
if (transaction->state != TRANSACTION_OPEN)
die("BUG: update_reflog called for transaction that is not
open");
update = add_update(transaction, refname, UPDATE_LOG);
+ update->flags = flags;
+ for (i = 0; i < transaction->nr - 1; i++) {
+ if (transaction->updates[i]->update_type != UPDATE_LOG)
+ continue;
+ if (!strcmp(transaction->updates[i]->refname,
+ update->refname)) {
+ update->flags |= UPDATE_REFLOG_NOLOCK;
+ update->orig_update = transaction->updates[i];
+ break;
+ }
+ }
+ if (!(update->flags & UPDATE_REFLOG_NOLOCK))
+ update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
+
hashcpy(update->new_sha1, new_sha1);
hashcpy(update->old_sha1, old_sha1);
update->reflog_fd = -1;
@@ -3639,7 +3661,6 @@ int transaction_update_reflog(struct transaction
*transaction,
}
if (msg)
update->msg = xstrdup(msg);
- update->flags = flags;
return 0;
}
@@ -3822,10 +3843,15 @@ int transaction_commit(struct transaction *transaction,
if (update->update_type != UPDATE_LOG)
continue;
+ if (update->flags & UPDATE_REFLOG_NOLOCK) {
+ update->reflog_fd = update->orig_update->reflog_fd;
+ update->reflog_lock = update->orig_update->reflog_lock;
+ continue;
+ }
update->reflog_fd = hold_lock_file_for_append(
- &update->reflog_lock,
+ update->reflog
[PATCH 10/15] refs.c: allow multiple reflog updates during a single transaction
Allow to make multiple reflog updates to the same ref during a transaction.
This means we only need to lock the reflog once, during the first update
that touches the reflog, and that all further updates can just write the
reflog entry since the reflog is already locked.
This allows us to write code such as:
t = transaction_begin()
transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
loop-over-somehting...
transaction_reflog_update(t, "foo", 0, );
transaction_commit(t)
where we first truncate the reflog and then build the new content one line at a
time.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 46 +-
1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index c431088..12ff916 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,12 @@ static unsigned char refname_disposition[256] = {
*/
#define REF_ISPRUNING 0x0100
/*
+ * Only the first reflog update needs to lock the reflog file. Further updates
+ * just use the lock taken by the first update.
+ */
+#define UPDATE_REFLOG_NOLOCK 0x0200
+
+/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
* not legal. It is legal if it is something reasonable to have under
@@ -3389,7 +3395,7 @@ enum transaction_update_type {
UPDATE_LOG = 1
};
-/**
+/*
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
* while locking the ref, set have_old to 1 and set old_sha1 to the
@@ -3399,7 +3405,7 @@ struct ref_update {
enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
- int flags; /* REF_NODEREF? */
+ int flags; /* REF_NODEREF? or private flags */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
int type;
@@ -3407,8 +3413,9 @@ struct ref_update {
/* used by reflog updates */
int reflog_fd;
- struct lock_file reflog_lock;
+ struct lock_file *reflog_lock;
char *committer;
+ struct ref_update *orig_update; /* For UPDATE_REFLOG_NOLOCK */
const char refname[FLEX_ARRAY];
};
@@ -3489,12 +3496,27 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
struct strbuf *err)
{
struct ref_update *update;
+ int i;
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: update_reflog called for transaction that is not "
"open");
update = add_update(transaction, refname, UPDATE_LOG);
+ update->flags = flags;
+ for (i = 0; i < transaction->nr - 1; i++) {
+ if (transaction->updates[i]->update_type != UPDATE_LOG)
+ continue;
+ if (!strcmp(transaction->updates[i]->refname,
+ update->refname)) {
+ update->flags |= UPDATE_REFLOG_NOLOCK;
+ update->orig_update = transaction->updates[i];
+ break;
+ }
+ }
+ if (!(update->flags & UPDATE_REFLOG_NOLOCK))
+ update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
+
hashcpy(update->new_sha1, new_sha1);
hashcpy(update->old_sha1, old_sha1);
update->reflog_fd = -1;
@@ -3510,7 +3532,6 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
}
if (msg)
update->msg = xstrdup(msg);
- update->flags = flags;
return 0;
}
@@ -3696,10 +3717,15 @@ int transaction_commit(struct ref_transaction
*transaction,
if (update->update_type != UPDATE_LOG)
continue;
+ if (update->flags & UPDATE_REFLOG_NOLOCK) {
+ update->reflog_fd = update->orig_update->reflog_fd;
+ update->reflog_lock = update->orig_update->reflog_lock;
+ continue;
+ }
update->reflog_fd = hold_lock_file_for_append(
- &update->reflog_lock,
+ update->reflog_lock,
git_path("logs/%s", update->refname),
- 0);
+ LOCK_NODEREF);
if (update->reflog_fd < 0) {
const char *str = "Cannot lock reflog for '%s'. %s";
@@ -3765,7 +3791,7 @@ int transaction_commit(struct ref_transaction
*transaction,
ftruncate(update->reflog_fd, 0)) {
error("Could not truncate reflog: %s. %s",
update->refname, strerror(errno));
- rollback_lock_file(&update->reflog_lock);
+
