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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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);
 		}