From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 dev.tarantool.org (Postfix) with ESMTPS id 9455C46970E for ; Sun, 19 Jan 2020 20:32:48 +0300 (MSK) References: <31e884f2b6d2ab7b222229f1b204fa067d63ae65.1579211601.git.v.shpilevoy@tarantool.org> <20200117074552.GD24940@atlas> From: Vladislav Shpilevoy Message-ID: Date: Sun, 19 Jan 2020 18:32:47 +0100 MIME-Version: 1.0 In-Reply-To: <20200117074552.GD24940@atlas> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 2/3] fiber: destroy fiber.storage created by iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org Hi! Thanks for the review! >> +/** >> + * Stop the current fiber after a request is executed to make it >> + * possible to reuse the fiber for a next request. On_stop >> + * triggers remove all request-specific data from there. >> + */ >> +static inline void >> +tx_fiber_on_stop() >> +{ >> + fiber_on_stop(fiber()); >> +} >> + >> static void >> tx_process_disconnect(struct cmsg *m) >> { >> @@ -1335,6 +1351,7 @@ tx_process_disconnect(struct cmsg *m) >> if (! rlist_empty(&session_on_disconnect)) { >> tx_fiber_init(con->session, 0); >> session_run_on_disconnect_triggers(con->session); >> + tx_fiber_on_stop(); >> } >> } > > Why did you have to add so many invocation points to > fiber_on_stop() rather than simply adding fiber_on_stop invocation to > fiber_pool.c? We already discussed that in the previous patch version. We decided to move cleanup to iproto.cc, because it depends on when a request ends. Fiber pool knows nothing about requests. Iproto.cc is request processing layer, and this is the right place for request data destruction. >> static void >> fiber_recycle(struct fiber *fiber); >> >> @@ -856,8 +869,7 @@ fiber_loop(MAYBE_UNUSED void *data) >> assert(f != fiber); >> fiber_wakeup(f); >> } >> - if (! rlist_empty(&fiber->on_stop)) >> - trigger_run(&fiber->on_stop, fiber); >> + fiber_on_stop(fiber); > > This was an attempt to optimize a non-inline function call > for the most common case. > > I would move this !rlist_empty check to fiber_on_stop and add a > comment why we explicitly check for the list first. I doubt it really helps, but ok. ================================================================================ diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index 634b3d1b0..354749549 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -328,6 +328,12 @@ fiber_attr_getstacksize(struct fiber_attr *fiber_attr) void fiber_on_stop(struct fiber *f) { + /* + * The most common case is when the list is empty. Do an + * inlined check before calling trigger_run(). + */ + if (rlist_empty(&f->on_stop)) + return; if (trigger_run(&f->on_stop, f) != 0) panic("On_stop triggers can't fail"); /*