From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6674B46970E for ; Mon, 20 Jan 2020 10:22:36 +0300 (MSK) Received: by mail-lj1-f196.google.com with SMTP id u1so32698945ljk.7 for ; Sun, 19 Jan 2020 23:22:36 -0800 (PST) Date: Mon, 20 Jan 2020 10:22:34 +0300 From: Konstantin Osipov Message-ID: <20200120072234.GB19835@atlas> References: <31e884f2b6d2ab7b222229f1b204fa067d63ae65.1579211601.git.v.shpilevoy@tarantool.org> <20200117074552.GD24940@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org * Vladislav Shpilevoy [20/01/20 09:59]: > > 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. True, but since you abstracted out the destruction via an opaque trigger, why not move the invocation of the trigger to fiber pool? fiber pool has most knowledge about fiber life cycle, so it seems natural to invoke the triggers in it - it will tie the *timing* to fiber pool, but not what's going on inside the trigger. Thoughts? > > 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"); > /* thanks! -- Konstantin Osipov, Moscow, Russia