From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9036B283DF for ; Thu, 11 Apr 2019 12:10:03 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qk61IKQ8awZR for ; Thu, 11 Apr 2019 12:10:03 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CB02E2B9C9 for ; Thu, 11 Apr 2019 12:10:02 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/1] swim: keep encoded round message cached References: <573c97faff11b387566fd449c1d1dcbe3e644351.1554893660.git.v.shpilevoy@tarantool.org> <20190411135150.GA5480@chai> From: Vladislav Shpilevoy Message-ID: Date: Thu, 11 Apr 2019 19:09:59 +0300 MIME-Version: 1.0 In-Reply-To: <20190411135150.GA5480@chai> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Konstantin Osipov On 11/04/2019 16:51, Konstantin Osipov wrote: > * Vladislav Shpilevoy [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); }