[tarantool-patches] Re: [PATCH] Output of fiber.info will contain only non-idle fibers

[Георгий Кириченко]

On Tuesday, July 23, 2019 10:56:43 PM MSK Konstantin Osipov wrote:
> * Maria K <marianneliash at gmail.com> [19/07/23 21:01]:
> > The output used to be too cluttered due to idle ones.
> > 
> > Closes #4235
> 
> @kyukhin, first, please I don't get how does this get scheduled to a
> milestone? How does this follow triage guidelines?
> 
> Please don't schedule anything that is not a priority, even if
> it's a noob issue, since it takes time of everyone involved.
I think anybody is free to send a patch to the public tarantool mailing list 
despite the issue milestone (if they is not bound by employee duties). Also it 
was a 'good first issue' ticket to start a candidate on boarding.
> 
> fiber.info() already doesn't show anything from cord->dead list.
> Fibers which are stuck in a pool are performing application-level
> code, even if it's a built in pool, so contribute valuable
> information to fiber.info() output. Besides, it's always easy to
> filter out any class of fibers with luafun.
It is not easy to filter out such fibers. In the other hand tx fiber pool is 'a 
hack' to spare some fiber structures between invocations. So fiber pool cached 
fibers could/should be threatened as dead ones.
> 
> Finally, there are other types of pools -- an application-level pool
> in Lua will have lots of idle fibers in it.
An application level fiber pool uses some user-defined condition with exactly-
defied meaning and state. And it isn't the same as the tx fiber pool. An 
appplication fiber (in pool or not) is the resource managed by user while tx 
fiber pool is not. And I see no point in seeing an idle fiber from tx fiber pool.
> 
> In other words, this is an partial fix of a raw feature
> request.
> 
> Tarantool instrumentation sucks, but it doesn't mean it should be
> patched by quick hacks here and there.
> 
> A nice and general solution would be to compress mostly identical
> fiber.info() entries. But I guess it's not a noob task.
I didn't find your suggestion solution nice and general in case of filtering 
idle fibers out.
> 
> > ---
> > 
> >  src/lib/core/fiber.c      |  3 ++-
> >  src/lib/core/fiber.h      | 10 ++++++++++
> >  src/lib/core/fiber_pool.c |  2 ++
> >  3 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> > index ce90f930c..b1d7a5be2 100644
> > --- a/src/lib/core/fiber.c
> > +++ b/src/lib/core/fiber.c
> > @@ -1411,7 +1411,8 @@ int fiber_stat(fiber_stat_cb cb, void *cb_ctx)
> > 
> >   struct cord *cord = cord();
> >   int res;
> >   rlist_foreach_entry(fiber, &cord->alive, link) {
> > 
> > - res = cb(fiber, cb_ctx);
> > + if (!fiber_is_idle(fiber))
> > + res = cb(fiber, cb_ctx);
> > 
> >   if (res != 0)
> >   return res;
> >   }
> > 
> > diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> > index fb168e25e..d05132a8d 100644
> > --- a/src/lib/core/fiber.h
> > +++ b/src/lib/core/fiber.h
> > @@ -97,6 +97,10 @@ enum {
> > 
> >   * This flag is set when fiber uses custom stack size.
> >   */
> >   FIBER_CUSTOM_STACK = 1 << 5,
> > 
> > + /*
> > + *
> > + */
> > + FIBER_IS_IDLE = 1 << 6,
> > 
> >   FIBER_DEFAULT_FLAGS = FIBER_IS_CANCELLABLE
> >  
> >  };
> > 
> > @@ -620,6 +624,12 @@ fiber_is_dead(struct fiber *f)
> > 
> >   return f->flags & FIBER_IS_DEAD;
> >  
> >  }
> > 
> > +static inline bool
> > +fiber_is_idle(struct fiber *f)
> > +{
> > + return f->flags & FIBER_IS_IDLE;
> > +}
> > +
> > 
> >  typedef int (*fiber_stat_cb)(struct fiber *f, void *ctx);
> >  
> >  int
> > 
> > diff --git a/src/lib/core/fiber_pool.c b/src/lib/core/fiber_pool.c
> > index 77f89c9fa..c04141e63 100644
> > --- a/src/lib/core/fiber_pool.c
> > +++ b/src/lib/core/fiber_pool.c
> > 
> > @@ -72,8 +72,10 @@ restart:
> >   * Add the fiber to the front of the list, so that
> >   * it is most likely to get scheduled again.
> >   */
> > 
> > + f->flags |= FIBER_IS_IDLE;
> > 
> >   rlist_add_entry(&pool->idle, fiber(), state);
> >   fiber_yield();
> > 
> > + f->flags &= ~FIBER_IS_IDLE;
> > 
> >   goto restart;
> >   }
> >   pool->size--;

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190725/cb57ef79/attachment.sig>