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

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

[Maria K]

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

The output used to be too cluttered due to idle ones.

Closes #4235
---
 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--;
-- 
2.21.0

[-- Attachment #2: Type: text/html, Size: 2073 bytes --]

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

[Konstantin Osipov]

* Maria K <marianneliash@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.

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.

Finally, there are other types of pools -- an application-level pool
in Lua will have lots of idle fibers in it. 

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.

> ---
>  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--;
> -- 
> 2.21.0

-- 
Konstantin Osipov, Moscow, Russia

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

[Kirill Yukhin]

Hello,

On 23 Jul 22:56, Konstantin Osipov wrote:
> * Maria K <marianneliash@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? 

I won't ever discuss any internal documents publicly, sorry.
I believe this contradicts ethitcs. That is second time you doing
this, please stop.

> Please don't schedule anything that is not a priority, even if
> it's a noob issue, since it takes time of everyone involved.

That was a customer request. If you'd like not to review the patch -
feel free to just skip it, we'll find another reviewer.

--
Regards, Kirill Yukhin

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

[Konstantin Osipov]

* Kirill Yukhin <kyukhin@tarantool.org> [19/07/24 11:03]:

> > * Maria K <marianneliash@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? 
> 
> I won't ever discuss any internal documents publicly, sorry.
> I believe this contradicts ethitcs. That is second time you doing
> this, please stop.

Well, I guess then it is time to make  SOP public.

How do you expect me to participate in it as a community member if
I don't share how the project is run and direction it takes? 

I've been mentioning this before -- things will have to become more
open, including defining the product roadmap and what features get
in, if we're to switch to a community driven model.

You can't expect one contributor party to drive the roadmap and
another merely do the review work.

> 
> > Please don't schedule anything that is not a priority, even if
> > it's a noob issue, since it takes time of everyone involved.
> 
> That was a customer request. If you'd like not to review the patch -
> feel free to just skip it, we'll find another reviewer.

Well, then it has to be well defined- I pointed out obvious
issues with this change. 

PS And no, TTLing reviews is also a wrong idea. One or two bad
patches getting in like that and it will be easier for me to fork
and cherry-pick changes than provide any reviews and work on a
common trunk.

-- 
Konstantin Osipov, Moscow, Russia

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

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

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

On Tuesday, July 23, 2019 10:56:43 PM MSK Konstantin Osipov wrote:
> * Maria K <marianneliash@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--;


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

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

[Konstantin Osipov]

* Георгий Кириченко <georgy@tarantool.org> [19/07/25 10:05]:
> On Tuesday, July 23, 2019 10:56:43 PM MSK Konstantin Osipov wrote:
> > * Maria K <marianneliash@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.

The reason it was a good first issue is that the problem was
wrongly defined in the first place. As defined, it had a trivial
solution.

The mere fact we have a disagreement suggests the label was
applied incorrectly.

> > 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.

Well, I don't think everyone should be unconditionally deprived of
this data if someone can't write a single-line snippet with luafun. 

> > 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.

I disagree there is no point in seeing these fibers. They take up
fiber stack and contribute to the total list libev events/fibers the
scheduler has to deal with.

> > 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.

Another way of properly fixing it would be to more
aggressively/carefully
expire such fibers from the pool. If you look at the current idle
callback implementation there are a few flaws in it:
- there is a standalone idle callback, rather than fiber idle
  timeout, and the callback removes no more than 1 fiber per
  second
- when the idle callback wakes up a fiber, it doesn't necessarily
  die. It looks at the pool->output first, and if there are
  messages in the output, works on them. Which is apparently
  wrong, because there always messages in the list on a busy
  system, this doesn't mean the idle fiber should be the one to
  work them off.

*None* of this would be visible/bothering anybody if this
information was hidden from fiber.info().  

So this begs the question: why am I wasting time
discussing/explaining this, how did it suddenly become a priority,
which I asked in the beginning of this thread.



-- 
Konstantin Osipov, Moscow, Russia

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

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

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

On Thursday, July 25, 2019 12:26:40 PM MSK Konstantin Osipov wrote:
> * Георгий Кириченко <georgy@tarantool.org> [19/07/25 10:05]:
> > On Tuesday, July 23, 2019 10:56:43 PM MSK Konstantin Osipov wrote:
> > > * Maria K <marianneliash@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.
> 
> The reason it was a good first issue is that the problem was
> wrongly defined in the first place. As defined, it had a trivial
> solution.
> 
> The mere fact we have a disagreement suggests the label was
> applied incorrectly.
> 
> > > 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.
> 
> Well, I don't think everyone should be unconditionally deprived of
> this data if someone can't write a single-line snippet with luafun.
There is no point to unwind such fibers stacks which costs a lot as well as 
produces a lot of lua garbage.
> 
> > > 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.
> I disagree there is no point in seeing these fibers. They take up
> fiber stack and contribute to the total list libev events/fibers the
> scheduler has to deal with.
If you checked the code you could see that such fibers do not consume libev 
events and participated only in alive list without any scheduling duties. Such 
fibers use only fiber stack which is already accounted using memory statistics.
> 
> > > 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.
> 
> Another way of properly fixing it would be to more
> aggressively/carefully
This makes fiber pool worthless because fiber pool has only two purposes: to 
limit the amount of fibers and to spare some resources. I would like to repeat: 
a fiber cached in a fiber pool is a dead fiber without any state.
> expire such fibers from the pool.
> If you look at the current idle
> callback implementation there are a few flaws in it:
> - there is a standalone idle callback, rather than fiber idle
>   timeout, and the callback removes no more than 1 fiber per
>   second
> - when the idle callback wakes up a fiber, it doesn't necessarily
>   die. It looks at the pool->output first, and if there are
>   messages in the output, works on them. Which is apparently
>   wrong, because there always messages in the list on a busy
>   system, this doesn't mean the idle fiber should be the one to
>   work them off.
Your were right only in case when a system has a stable rps what is definitely 
not the common case. And if a system has no idle fibers (in case of stable rps) 
there is no reason to have a fiber pool.
> 
> *None* of this would be visible/bothering anybody if this
> information was hidden from fiber.info().
And we want to hide fiber pool internals from user by filtering idle (dead, 
state-less, unused, resource-less) fibers out.
> 
> So this begs the question: why am I wasting time
> discussing/explaining this, how did it suddenly become a priority,
> which I asked in the beginning of this thread.
It was only your decision to discuss this as you are free to left it out. As 
anybody else is free to submit a patch.

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