[tarantool-patches] [PATCH 1/1] swim: keep encoded round message cached

[tarantool-patches] [PATCH 1/1] swim: keep encoded round message cached

[Vladislav Shpilevoy]

During a SWIM round a message is being handed out consisting of
at most 4 sections. Parts of the message change rarely along with
a member attribute update, or with removal of a member. So it is
possible to cache the message and send it during several round
steps in a row. Or even do not rebuild it the whole round.

Part of #3234
---
Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-3234-swim-cached-msg
Issue: https://github.com/tarantool/tarantool/issues/3234

 src/lib/swim/swim.c    | 24 ++++++++++++++++++++----
 src/lib/swim/swim_io.h |  7 +++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index c64b8df3a..8453295a5 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -417,6 +417,13 @@ struct swim {
 	struct rlist dissemination_queue;
 };
 
+/** Reset cached round message on any change of any member. */
+static inline void
+cached_round_msg_invalidate(struct swim *swim)
+{
+	swim_packet_create(&swim->round_step_task.packet);
+}
+
 /** Put the member into a list of ACK waiters. */
 static void
 swim_wait_ack(struct swim *swim, struct swim_member *member)
@@ -456,6 +463,7 @@ static void
 swim_on_member_update(struct swim *swim, struct swim_member *member)
 {
 	member->unacknowledged_pings = 0;
+	cached_round_msg_invalidate(swim);
 	swim_register_event(swim, member);
 }
 
@@ -588,6 +596,7 @@ swim_delete_member(struct swim *swim, struct swim_member *member)
 	mh_int_t rc = mh_swim_table_find(swim->members, key, NULL);
 	assert(rc != mh_end(swim->members));
 	mh_swim_table_del(swim->members, rc, NULL);
+	cached_round_msg_invalidate(swim);
 	rlist_del_entry(member, in_round_queue);
 
 	/* Failure detection component. */
@@ -836,8 +845,11 @@ swim_encode_dissemination(struct swim *swim, struct swim_packet *packet)
 
 /** Encode SWIM components into a UDP packet. */
 static void
-swim_encode_round_msg(struct swim *swim, struct swim_packet *packet)
+swim_encode_round_msg(struct swim *swim)
 {
+	if (swim_packet_body_size(&swim->round_step_task.packet) > 0)
+		return;
+	struct swim_packet *packet = &swim->round_step_task.packet;
 	swim_packet_create(packet);
 	char *header = swim_packet_alloc(packet, 1);
 	int map_size = 0;
@@ -872,6 +884,7 @@ swim_decrease_event_ttl(struct swim *swim)
 				 tmp) {
 		if (--member->status_ttl == 0) {
 			rlist_del_entry(member, in_dissemination_queue);
+			cached_round_msg_invalidate(swim);
 			if (member->status == MEMBER_LEFT)
 				swim_delete_member(swim, member);
 		}
@@ -901,8 +914,7 @@ swim_begin_step(struct ev_loop *loop, struct ev_timer *t, int events)
 	 */
 	if (rlist_empty(&swim->round_queue))
 		return;
-
-	swim_encode_round_msg(swim, &swim->round_step_task.packet);
+	swim_encode_round_msg(swim);
 	struct swim_member *m =
 		rlist_first_entry(&swim->round_queue, struct swim_member,
 				  in_round_queue);
@@ -1559,7 +1571,6 @@ swim_delete(struct swim *swim)
 	swim_scheduler_destroy(&swim->scheduler);
 	swim_ev_timer_stop(loop(), &swim->round_tick);
 	swim_ev_timer_stop(loop(), &swim->wait_ack_tick);
-	swim_task_destroy(&swim->round_step_task);
 	mh_int_t node;
 	mh_foreach(swim->members, node) {
 		struct swim_member *m =
@@ -1570,6 +1581,11 @@ swim_delete(struct swim *swim)
 		rlist_del_entry(m, in_dissemination_queue);
 		swim_member_delete(m);
 	}
+	/*
+	 * Destroy the task after members - otherwise they would
+	 * try to invalidate the already destroyed task.
+	 */
+	swim_task_destroy(&swim->round_step_task);
 	wait_ack_heap_destroy(&swim->wait_ack_heap);
 	mh_swim_table_delete(swim->members);
 	free(swim->shuffled);
diff --git a/src/lib/swim/swim_io.h b/src/lib/swim/swim_io.h
index a6032127d..30016f904 100644
--- a/src/lib/swim/swim_io.h
+++ b/src/lib/swim/swim_io.h
@@ -126,6 +126,13 @@ swim_packet_alloc(struct swim_packet *packet, int size)
 	return res;
 }
 
+/** Size of the packet body. Meta is not counted. */
+static inline int
+swim_packet_body_size(const struct swim_packet *packet)
+{
+	return packet->pos - packet->body;
+}
+
 /** Initialize @a packet, reserve some space for meta. */
 void
 swim_packet_create(struct swim_packet *packet);
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] Re: [PATCH 1/1] swim: keep encoded round message cached

[Vladislav Shpilevoy]

Sorry, forgot to attach one more hunk:

diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index 8453295a5..2839f5681 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -713,6 +713,7 @@ swim_new_round(struct swim *swim)
 	/* -1 for self. */
 	say_verbose("SWIM %d: start a new round with %d members", swim_fd(swim),
 		    size - 1);
+	cached_round_msg_invalidate(swim);
 	swim_shuffle_members(swim);
 	rlist_create(&swim->round_queue);
 	for (int i = 0; i < size; ++i) {

[tarantool-patches] Re: [PATCH 1/1] swim: keep encoded round message cached

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/10 17:15]:
> During a SWIM round a message is being handed out consisting of
> at most 4 sections. Parts of the message change rarely along with
> a member attribute update, or with removal of a member. So it is
> possible to cache the message and send it during several round
> steps in a row. Or even do not rebuild it the whole round.
> 
> Part of #3234
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-3234-swim-cached-msg
> Issue: https://github.com/tarantool/tarantool/issues/3234
> 
>  src/lib/swim/swim.c    | 24 ++++++++++++++++++++----
>  src/lib/swim/swim_io.h |  7 +++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
> index c64b8df3a..8453295a5 100644
> --- a/src/lib/swim/swim.c
> +++ b/src/lib/swim/swim.c
> @@ -417,6 +417,13 @@ struct swim {
>  	struct rlist dissemination_queue;
>  };
>  
> +/** Reset cached round message on any change of any member. */
> +static inline void
> +cached_round_msg_invalidate(struct swim *swim)
> +{
> +	swim_packet_create(&swim->round_step_task.packet);
> +}

Are you going to add anything to this method? If not, please get
rid of it and inline swim_packet_create() to all relevant places,
with an appropriate comment.

>  swim_on_member_update(struct swim *swim, struct swim_member *member)
>  {
>  	member->unacknowledged_pings = 0;
> +	cached_round_msg_invalidate(swim);
>  	swim_register_event(swim, member);

Please move the call to cached_round_msg_invalidate() to
swim_register_event(). Actually the three members -
in_dissemination_queue, status_ttl and cached message - have
interdependent life cycle. There are two major events to account
for: 
    - whenever you reset status_ttl you need to invalidate the
      cache and ensure the member is registered in the
      dissemination queue.
    - whenever status_ttl drops to zero the member is deleted from
      the dissemination queue and the cached message can be
      deleted as well.

You have two relevant places for these two events:
swim_register_event and swim_decrease_event_ttl().

It should be sufficient to add cache invalidation to them, plus
initialize the cache in the constructor and destroy in the
destructor.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 1/1] swim: keep encoded round message cached

[Vladislav Shpilevoy]

On 11/04/2019 16:51, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/10 17:15]:
>> During a SWIM round a message is being handed out consisting of
>> at most 4 sections. Parts of the message change rarely along with
>> a member attribute update, or with removal of a member. So it is
>> possible to cache the message and send it during several round
>> steps in a row. Or even do not rebuild it the whole round.
>>
>> Part of #3234
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-3234-swim-cached-msg
>> Issue: https://github.com/tarantool/tarantool/issues/3234
>>
>>  src/lib/swim/swim.c    | 24 ++++++++++++++++++++----
>>  src/lib/swim/swim_io.h |  7 +++++++
>>  2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
>> index c64b8df3a..8453295a5 100644
>> --- a/src/lib/swim/swim.c
>> +++ b/src/lib/swim/swim.c
>> @@ -417,6 +417,13 @@ struct swim {
>>  	struct rlist dissemination_queue;
>>  };
>>  
>> +/** Reset cached round message on any change of any member. */
>> +static inline void
>> +cached_round_msg_invalidate(struct swim *swim)
>> +{
>> +	swim_packet_create(&swim->round_step_task.packet);
>> +}
> 
> Are you going to add anything to this method? If not, please get
> rid of it and inline swim_packet_create() to all relevant places,
> with an appropriate comment.

I am not going to change it now, but

1) I (or anyone else) can change my (their) mind in future. Even
   during SWIM development we already did it. I have this patch
   since the SWIM implementation was multi-packet, and invalidation
   of the cache was different.

2) This logic of recreating the packet is too internal to be
   inlined in all the usage places. I would rather comment this
   function and logic in one encapsulated place, and would just
   call it.

So if you do not have better objectives, I would like to keep it
as a special function. Although, I renamed it with a prefix 'swim_'
to be consistent in the naming policy.

> 
>>  swim_on_member_update(struct swim *swim, struct swim_member *member)
>>  {
>>  	member->unacknowledged_pings = 0;
>> +	cached_round_msg_invalidate(swim);
>>  	swim_register_event(swim, member);
> 
> Please move the call to cached_round_msg_invalidate() to
> swim_register_event().

You are right, it looks better. Honestly, firstly I did exactly
this, but inexplicably decided to change it to the way you
reviewed. I've fixed that, and added a comment to round_task.

> Actually the three members -
> in_dissemination_queue, status_ttl and cached message - have
> interdependent life cycle. There are two major events to account
> for: 
>     - whenever you reset status_ttl you need to invalidate the
>       cache and ensure the member is registered in the
>       dissemination queue.
>     - whenever status_ttl drops to zero the member is deleted from
>       the dissemination queue and the cached message can be
>       deleted as well.
> 
> You have two relevant places for these two events:
> swim_register_event and swim_decrease_event_ttl().
> 
> It should be sufficient to add cache invalidation to them, plus
> initialize the cache in the constructor and destroy in the
> destructor.

Yes, totally right. With a couple of additions:

1) If a member is just deleted, and it has zero TTL, and
   is not kept in the dissemination queue, then the cache
   should be invalidated as well. Because that member could
   be stored in the anti-entropy section.

2) After each round I should invalidate the cache, because
   anti-entropy section should be rebuild with new random
   members.

I had already done these two comments before you gave them,
and I answered them here just in case.

See incremental diff below. Please, do not forget to respond
to the mail with new comments, or with ok to push, because I
did not see anything similar with an auto-push permission in
your comments.

===============================================================

diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index 2839f5681..cb888a168 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -364,7 +364,10 @@ struct swim {
 	 * Single round step task. It is impossible to have
 	 * multiple round steps in the same SWIM instance at the
 	 * same time, so it is single and preallocated per SWIM
-	 * instance.
+	 * instance. Note, that the task's packet once built at
+	 * the beginning of a round is reused during the round
+	 * without rebuilding on each step. But packet rebuild can
+	 * be triggered by any update of any member.
 	 */
 	struct swim_task round_step_task;
 	/**
@@ -419,7 +422,7 @@ struct swim {
 
 /** Reset cached round message on any change of any member. */
 static inline void
-cached_round_msg_invalidate(struct swim *swim)
+swim_cached_round_msg_invalidate(struct swim *swim)
 {
 	swim_packet_create(&swim->round_step_task.packet);
 }
@@ -453,6 +456,7 @@ swim_register_event(struct swim *swim, struct swim_member *member)
 				     in_dissemination_queue);
 	}
 	member->status_ttl = mh_size(swim->members);
+	swim_cached_round_msg_invalidate(swim);
 }
 
 /**
@@ -463,7 +467,6 @@ static void
 swim_on_member_update(struct swim *swim, struct swim_member *member)
 {
 	member->unacknowledged_pings = 0;
-	cached_round_msg_invalidate(swim);
 	swim_register_event(swim, member);
 }
 
@@ -596,7 +599,7 @@ swim_delete_member(struct swim *swim, struct swim_member *member)
 	mh_int_t rc = mh_swim_table_find(swim->members, key, NULL);
 	assert(rc != mh_end(swim->members));
 	mh_swim_table_del(swim->members, rc, NULL);
-	cached_round_msg_invalidate(swim);
+	swim_cached_round_msg_invalidate(swim);
 	rlist_del_entry(member, in_round_queue);
 
 	/* Failure detection component. */
@@ -713,7 +716,7 @@ swim_new_round(struct swim *swim)
 	/* -1 for self. */
 	say_verbose("SWIM %d: start a new round with %d members", swim_fd(swim),
 		    size - 1);
-	cached_round_msg_invalidate(swim);
+	swim_cached_round_msg_invalidate(swim);
 	swim_shuffle_members(swim);
 	rlist_create(&swim->round_queue);
 	for (int i = 0; i < size; ++i) {
@@ -885,7 +888,7 @@ swim_decrease_event_ttl(struct swim *swim)
 				 tmp) {
 		if (--member->status_ttl == 0) {
 			rlist_del_entry(member, in_dissemination_queue);
-			cached_round_msg_invalidate(swim);
+			swim_cached_round_msg_invalidate(swim);
 			if (member->status == MEMBER_LEFT)
 				swim_delete_member(swim, member);
 		}

[tarantool-patches] Re: [PATCH 1/1] swim: keep encoded round message cached

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/11 19:13]:

OK to push.

> > Are you going to add anything to this method? If not, please get
> > rid of it and inline swim_packet_create() to all relevant places,
> > with an appropriate comment.
> 
> I am not going to change it now, but
> 
> 1) I (or anyone else) can change my (their) mind in future. Even
>    during SWIM development we already did it. I have this patch
>    since the SWIM implementation was multi-packet, and invalidation
>    of the cache was different.
> 
> 2) This logic of recreating the packet is too internal to be
>    inlined in all the usage places. I would rather comment this
>    function and logic in one encapsulated place, and would just
>    call it.

They can easily re-introduce this function when necessary.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 1/1] swim: keep encoded round message cached

[Vladislav Shpilevoy]

Pushed to the master.

On 11/04/2019 19:24, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/11 19:13]:
> 
> OK to push.
> 
>>> Are you going to add anything to this method? If not, please get
>>> rid of it and inline swim_packet_create() to all relevant places,
>>> with an appropriate comment.
>>
>> I am not going to change it now, but
>>
>> 1) I (or anyone else) can change my (their) mind in future. Even
>>    during SWIM development we already did it. I have this patch
>>    since the SWIM implementation was multi-packet, and invalidation
>>    of the cache was different.
>>
>> 2) This logic of recreating the packet is too internal to be
>>    inlined in all the usage places. I would rather comment this
>>    function and logic in one encapsulated place, and would just
>>    call it.
> 
> They can easily re-introduce this function when necessary.
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
>