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

[Vladislav Shpilevoy <v.shpilevoy@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");
 	/*