Re: [Tarantool-patches] [PATCH v2 2/3] fiber: destroy fiber.storage created by iproto

[Georgy Kirichenko <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On Monday, 20 January 2020 10:22:34 MSK Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [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 agree with Kostja's comment, if a fiber pool is only an optimization then 
there should not be any visible difference between code invocation inside a 
standalone fiber and a fiber pool member and this point includes fiber pool 
clearance.
> 
> > > 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!


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]