[Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Vladislav Shpilevoy]

TXN_IS_DONE was used in txn_commit() to finalize the transaction
in a case it is not finished yet. But this happens only not in
so common cases - during bootstrap and recovery. During normal
operation the transaction is always finished when WAL thread
returns it to TX thread after a disk write.

So this is a matter of journal, and should be solved here, not in
txn code with some crutch, especially in such a hot path place.

This commit makes so that after journal_write() the transaction is
always already done, i.e. txn_complete_async() was called. Nothing
changes for the normal operation mode except this is -1 'if'.
---
Branch: http://github.com/tarantool/tarantool/tree/gerold103/txn_commit-opt

I couldn't get any stable bench results on my machine. But seems
like this solution probably gives something about 1-2% perf
increase for pure 'insert' workload without anything else.

Need someone else to try too. Sergey Os.?

 src/box/box.cc    | 23 +++++++++++++++++++++--
 src/box/journal.c | 45 +--------------------------------------------
 src/box/journal.h | 11 -----------
 src/box/txn.c     |  6 +-----
 src/box/wal.c     |  2 +-
 5 files changed, 24 insertions(+), 63 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 871b0d976..ca061dc4f 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -323,6 +323,7 @@ recovery_journal_write(struct journal *base,
 {
 	struct recovery_journal *journal = (struct recovery_journal *) base;
 	entry->res = vclock_sum(journal->vclock);
+	journal_async_complete(base, entry);
 	return 0;
 }
 
@@ -330,8 +331,7 @@ static void
 recovery_journal_create(struct vclock *v)
 {
 	static struct recovery_journal journal;
-	journal_create(&journal.base, journal_no_write_async,
-		       journal_no_write_async_cb,
+	journal_create(&journal.base, NULL, txn_complete_async,
 		       recovery_journal_write);
 	journal.vclock = v;
 	journal_set(&journal.base);
@@ -2017,6 +2017,14 @@ engine_init()
 	box_set_vinyl_timeout();
 }
 
+static int
+bootstrap_journal_write(struct journal *base, struct journal_entry *entry)
+{
+	entry->res = 0;
+	journal_async_complete(base, entry);
+	return 0;
+}
+
 /**
  * Initialize the first replica of a new replica set.
  */
@@ -2438,6 +2446,14 @@ box_cfg_xc(void)
 		if (!cfg_geti("hot_standby") || checkpoint == NULL)
 			tnt_raise(ClientError, ER_ALREADY_RUNNING, cfg_gets("wal_dir"));
 	}
+	struct journal bootstrap_journal;
+	journal_create(&bootstrap_journal, NULL, txn_complete_async,
+		       bootstrap_journal_write);
+	journal_set(&bootstrap_journal);
+	auto bootstrap_journal_guard = make_scoped_guard([] {
+		journal_set(NULL);
+	});
+
 	bool is_bootstrap_leader = false;
 	if (checkpoint != NULL) {
 		/* Recover the instance from the local directory */
@@ -2450,6 +2466,9 @@ box_cfg_xc(void)
 	}
 	fiber_gc();
 
+	bootstrap_journal_guard.is_active = false;
+	assert(current_journal != &bootstrap_journal);
+
 	/*
 	 * Check for correct registration of the instance in _cluster
 	 * The instance won't exist in _cluster space if it is an
diff --git a/src/box/journal.c b/src/box/journal.c
index d84f960d5..f1e89aaa2 100644
--- a/src/box/journal.c
+++ b/src/box/journal.c
@@ -32,50 +32,7 @@
 #include <small/region.h>
 #include <diag.h>
 
-int
-journal_no_write_async(struct journal *journal,
-		       struct journal_entry *entry)
-{
-	(void)journal;
-	assert(false);
-
-	errno = EINVAL;
-	diag_set(SystemError, "write_async wrong context");
-
-	entry->res = -1;
-	return -1;
-}
-
-void
-journal_no_write_async_cb(struct journal_entry *entry)
-{
-	assert(false);
-
-	errno = EINVAL;
-	diag_set(SystemError, "write_async_cb wrong context");
-
-	entry->res = -1;
-}
-
-/**
- * Used to load from a memtx snapshot. LSN is not used,
- * but txn_commit() must work.
- */
-static int
-dummy_journal_write(struct journal *journal, struct journal_entry *entry)
-{
-	(void) journal;
-	entry->res = 0;
-	return 0;
-}
-
-static struct journal dummy_journal = {
-	.write_async	= journal_no_write_async,
-	.write_async_cb	= journal_no_write_async_cb,
-	.write		= dummy_journal_write,
-};
-
-struct journal *current_journal = &dummy_journal;
+struct journal *current_journal = NULL;
 
 struct journal_entry *
 journal_entry_new(size_t n_rows, struct region *region,
diff --git a/src/box/journal.h b/src/box/journal.h
index ebc5cb708..1a10e66c3 100644
--- a/src/box/journal.h
+++ b/src/box/journal.h
@@ -182,17 +182,6 @@ journal_create(struct journal *journal,
 	journal->write		= write;
 }
 
-/**
- * A stub to issue an error in case if asynchronous
- * write is diabled in the backend.
- */
-extern int
-journal_no_write_async(struct journal *journal,
-		       struct journal_entry *entry);
-
-extern void
-journal_no_write_async_cb(struct journal_entry *entry);
-
 static inline bool
 journal_is_initialized(struct journal *journal)
 {
diff --git a/src/box/txn.c b/src/box/txn.c
index 123520166..e86a03447 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -652,11 +652,7 @@ txn_commit(struct txn *txn)
 		diag_log();
 		return -1;
 	}
-
-	if (!txn_has_flag(txn, TXN_IS_DONE)) {
-		txn->signature = req->res;
-		txn_complete(txn);
-	}
+	assert(txn_has_flag(txn, TXN_IS_DONE));
 
 	int res = txn->signature >= 0 ? 0 : -1;
 	if (res != 0) {
diff --git a/src/box/wal.c b/src/box/wal.c
index 74cc74684..9ee1a4e7f 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -1285,7 +1285,7 @@ wal_write_none_async(struct journal *journal,
 	vclock_merge(&writer->vclock, &vclock_diff);
 	vclock_copy(&replicaset.vclock, &writer->vclock);
 	entry->res = vclock_sum(&writer->vclock);
-
+	journal_async_complete(journal, entry);
 	return 0;
 }
 
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Vladislav Shpilevoy]

Also adding Sergey P.

Re: [Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Serge Petrenko]

04.07.2020 02:23, Vladislav Shpilevoy пишет:
> TXN_IS_DONE was used in txn_commit() to finalize the transaction
> in a case it is not finished yet. But this happens only not in
> so common cases - during bootstrap and recovery. During normal
> operation the transaction is always finished when WAL thread
> returns it to TX thread after a disk write.
>
> So this is a matter of journal, and should be solved here, not in
> txn code with some crutch, especially in such a hot path place.
>
> This commit makes so that after journal_write() the transaction is
> always already done, i.e. txn_complete_async() was called. Nothing
> changes for the normal operation mode except this is -1 'if'.
> ---
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/txn_commit-opt
>
> I couldn't get any stable bench results on my machine. But seems
> like this solution probably gives something about 1-2% perf
> increase for pure 'insert' workload without anything else.
>
> Need someone else to try too. Sergey Os.?
>
>   src/box/box.cc    | 23 +++++++++++++++++++++--
>   src/box/journal.c | 45 +--------------------------------------------
>   src/box/journal.h | 11 -----------
>   src/box/txn.c     |  6 +-----
>   src/box/wal.c     |  2 +-
>   5 files changed, 24 insertions(+), 63 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 871b0d976..ca061dc4f 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -323,6 +323,7 @@ recovery_journal_write(struct journal *base,
>   {
>   	struct recovery_journal *journal = (struct recovery_journal *) base;
>   	entry->res = vclock_sum(journal->vclock);
> +	journal_async_complete(base, entry);
>   	return 0;
>   }
>   
> @@ -330,8 +331,7 @@ static void
>   recovery_journal_create(struct vclock *v)
>   {
>   	static struct recovery_journal journal;
> -	journal_create(&journal.base, journal_no_write_async,
> -		       journal_no_write_async_cb,
> +	journal_create(&journal.base, NULL, txn_complete_async,
>   		       recovery_journal_write);
>   	journal.vclock = v;
>   	journal_set(&journal.base);
> @@ -2017,6 +2017,14 @@ engine_init()
>   	box_set_vinyl_timeout();
>   }
>   
> +static int
> +bootstrap_journal_write(struct journal *base, struct journal_entry *entry)
> +{
> +	entry->res = 0;
> +	journal_async_complete(base, entry);
> +	return 0;
> +}
> +
>   /**
>    * Initialize the first replica of a new replica set.
>    */
> @@ -2438,6 +2446,14 @@ box_cfg_xc(void)
>   		if (!cfg_geti("hot_standby") || checkpoint == NULL)
>   			tnt_raise(ClientError, ER_ALREADY_RUNNING, cfg_gets("wal_dir"));
>   	}
> +	struct journal bootstrap_journal;
> +	journal_create(&bootstrap_journal, NULL, txn_complete_async,
> +		       bootstrap_journal_write);
> +	journal_set(&bootstrap_journal);
> +	auto bootstrap_journal_guard = make_scoped_guard([] {
> +		journal_set(NULL);
> +	});
> +
>   	bool is_bootstrap_leader = false;
>   	if (checkpoint != NULL) {
>   		/* Recover the instance from the local directory */
> @@ -2450,6 +2466,9 @@ box_cfg_xc(void)
>   	}
>   	fiber_gc();
>   
> +	bootstrap_journal_guard.is_active = false;
> +	assert(current_journal != &bootstrap_journal);
> +
>   	/*
>   	 * Check for correct registration of the instance in _cluster
>   	 * The instance won't exist in _cluster space if it is an
> diff --git a/src/box/journal.c b/src/box/journal.c
> index d84f960d5..f1e89aaa2 100644
> --- a/src/box/journal.c
> +++ b/src/box/journal.c
> @@ -32,50 +32,7 @@
>   #include <small/region.h>
>   #include <diag.h>
>   
> -int
> -journal_no_write_async(struct journal *journal,
> -		       struct journal_entry *entry)
> -{
> -	(void)journal;
> -	assert(false);
> -
> -	errno = EINVAL;
> -	diag_set(SystemError, "write_async wrong context");
> -
> -	entry->res = -1;
> -	return -1;
> -}
> -
> -void
> -journal_no_write_async_cb(struct journal_entry *entry)
> -{
> -	assert(false);
> -
> -	errno = EINVAL;
> -	diag_set(SystemError, "write_async_cb wrong context");
> -
> -	entry->res = -1;
> -}
> -
> -/**
> - * Used to load from a memtx snapshot. LSN is not used,
> - * but txn_commit() must work.
> - */
> -static int
> -dummy_journal_write(struct journal *journal, struct journal_entry *entry)
> -{
> -	(void) journal;
> -	entry->res = 0;
> -	return 0;
> -}
> -
> -static struct journal dummy_journal = {
> -	.write_async	= journal_no_write_async,
> -	.write_async_cb	= journal_no_write_async_cb,
> -	.write		= dummy_journal_write,
> -};
> -
> -struct journal *current_journal = &dummy_journal;
> +struct journal *current_journal = NULL;
>   
>   struct journal_entry *
>   journal_entry_new(size_t n_rows, struct region *region,
> diff --git a/src/box/journal.h b/src/box/journal.h
> index ebc5cb708..1a10e66c3 100644
> --- a/src/box/journal.h
> +++ b/src/box/journal.h
> @@ -182,17 +182,6 @@ journal_create(struct journal *journal,
>   	journal->write		= write;
>   }
>   
> -/**
> - * A stub to issue an error in case if asynchronous
> - * write is diabled in the backend.
> - */
> -extern int
> -journal_no_write_async(struct journal *journal,
> -		       struct journal_entry *entry);
> -
> -extern void
> -journal_no_write_async_cb(struct journal_entry *entry);
> -
>   static inline bool
>   journal_is_initialized(struct journal *journal)
>   {
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 123520166..e86a03447 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -652,11 +652,7 @@ txn_commit(struct txn *txn)
>   		diag_log();
>   		return -1;
>   	}
> -
> -	if (!txn_has_flag(txn, TXN_IS_DONE)) {
> -		txn->signature = req->res;
> -		txn_complete(txn);
> -	}
> +	assert(txn_has_flag(txn, TXN_IS_DONE));
>   
>   	int res = txn->signature >= 0 ? 0 : -1;
>   	if (res != 0) {
> diff --git a/src/box/wal.c b/src/box/wal.c
> index 74cc74684..9ee1a4e7f 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -1285,7 +1285,7 @@ wal_write_none_async(struct journal *journal,
>   	vclock_merge(&writer->vclock, &vclock_diff);
>   	vclock_copy(&replicaset.vclock, &writer->vclock);
>   	entry->res = vclock_sum(&writer->vclock);
> -
> +	journal_async_complete(journal, entry);
>   	return 0;
>   }
>   

Thanks for the patch! Looks good.

Speaking of benchmarks, making replaces in hash index with wal_mode = 'none'
on release, I somehow got a 5% decrease in performance. However, it's really
hard to measure the difference, because results aren't stable.

With debug build and tree index, wal_mode='none', there is no noticeable 
difference.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Vladislav Shpilevoy]

Still need somebody to take a look at this. It would
help to make txn_commit more straightforward.

Sergey Os.? Cyrill G. maybe?

Re: [Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Cyrill Gorcunov]

On Mon, Jul 20, 2020 at 10:24:01PM +0200, Vladislav Shpilevoy wrote:
> Still need somebody to take a look at this. It would
> help to make txn_commit more straightforward.
> 
> Sergey Os.? Cyrill G. maybe?

The idea looks reasonable. Could you please rebase the patch
on current master once time permit? This flag comes from the
series where I've been reworking journal engine hunting the
nil dereference in parallel applier code and after discussion
with KostyaO we choose to make helper functions which would
trigger error in case if async write is called from bootstrap
and recover code.

Still removing code is always a good thing :-) I'll try to
rebase it myself tomorrow if you wont beat me on it today.

Re: [Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Cyrill Gorcunov]

On Tue, Jul 21, 2020 at 01:21:17AM +0300, Cyrill Gorcunov wrote:
> On Mon, Jul 20, 2020 at 10:24:01PM +0200, Vladislav Shpilevoy wrote:
> > Still need somebody to take a look at this. It would
> > help to make txn_commit more straightforward.
> > 
> > Sergey Os.? Cyrill G. maybe?
> 
> The idea looks reasonable. Could you please rebase the patch
> on current master once time permit? This flag comes from the
> series where I've been reworking journal engine hunting the
> nil dereference in parallel applier code and after discussion
> with KostyaO we choose to make helper functions which would
> trigger error in case if async write is called from bootstrap
> and recover code.
> 
> Still removing code is always a good thing :-) I'll try to
> rebase it myself tomorrow if you wont beat me on it today.

I've updated the patch locally. Vlad, lets postpone this
particular patch -- I'm about to change journal code again
for qsync reason and will grab your patch on top. Then will
send it in the batch. Sounds ok?

Re: [Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Vladislav Shpilevoy]

On 21.07.2020 10:45, Cyrill Gorcunov wrote:
> On Tue, Jul 21, 2020 at 01:21:17AM +0300, Cyrill Gorcunov wrote:
>> On Mon, Jul 20, 2020 at 10:24:01PM +0200, Vladislav Shpilevoy wrote:
>>> Still need somebody to take a look at this. It would
>>> help to make txn_commit more straightforward.
>>>
>>> Sergey Os.? Cyrill G. maybe?
>>
>> The idea looks reasonable. Could you please rebase the patch
>> on current master once time permit? This flag comes from the
>> series where I've been reworking journal engine hunting the
>> nil dereference in parallel applier code and after discussion
>> with KostyaO we choose to make helper functions which would
>> trigger error in case if async write is called from bootstrap
>> and recover code.
>>
>> Still removing code is always a good thing :-) I'll try to
>> rebase it myself tomorrow if you wont beat me on it today.
> 
> I've updated the patch locally. Vlad, lets postpone this
> particular patch -- I'm about to change journal code again
> for qsync reason and will grab your patch on top. Then will
> send it in the batch. Sounds ok?

I rebased the patch and added one more commit to make it work. See
the new email thread.

Re: [Tarantool-patches] [PATCH 1/1] txn: remove TXN_IS_DONE check from txn_commit()

[Cyrill Gorcunov]

On Wed, Jul 22, 2020 at 12:43:35AM +0200, Vladislav Shpilevoy wrote:
> 
> I rebased the patch and added one more commit to make it work. See
> the new email thread.

Thanks! Will review today.