Revert "more fiber notifications"

[twose]

Sorry i just saw this change now, and I have not seen the context of the discussion about it for the time being.
I don’t think we need more notify API, the status should be checked instead.
Correct me if I am wrong.
This revert 848b545 (#7293)

[bwoebi]

hm ... if a fiber is destroyed as the last reference to it is removed, won't it not do a fiber switch and thus not invoke the observer at all?

[trowski]

@bwoebi The fiber is switched into during destruction, so the watcher will still be triggered.

@morrisonlevi What is suggested here is how I originally described the observer API to you in R11. There is of course a timing difference: creation notifications are not available until the first switch, and destroy notifications come on the last switch rather than when memory is alloc/dealloc. Will that work for you? If so, we can drop these extra observers.

[krakjoe]

Not sure what the motivation for changing this is.

[twose]

Not sure what the motivation for changing this is.

@krakjoe If we can achieve our goals in a simpler way, why do we need more?
I'm just confused by the new API when I saw it for the first time, because I always thought we could check it through status.
Of course this is just my personal subjective feeling, It ok if you prefer more new specific APIs :)
I plan to use fiber notify API in Swoole, I just want to make things easier from the user's point of view.

[twose]

https://github.com/xdebug/xdebug/blob/master/src/base/base.c#L1098
Even Xdebug knows that it can get init and destory events by status.
zend_observer_fiber_init and zend_observer_fiber_destroy were complete noise to me.
I have made my point, but it seems that @krakjoe do not want to explain what the motivation for adding these API is. Maybe it's not friendly to me :)
I feel like you guys are being really easy on yourselves, unfortunately I don't feel that way.

[krakjoe]

#7292 and #7293 came about as a result of conversations with consumers of the observer API, and in the interest of completeness and flexibility. If you look at #7293 you will notice that several consumers of the API approved these changes.

That you don't understand why they are here, when there is other ways to achieve the same thing is not a good reason to revert commits approved by several other contributors.

You also said:

Of course this is just my personal subjective feeling, It ok if you prefer more new specific APIs :)

There doesn't seem to be a problem.

[twose]

@bwoebi The fiber is switched into during destruction, so the watcher will still be triggered.

@morrisonlevi What is suggested here is how I originally described the observer API to you in R11. There is of course a timing difference: creation notifications are not available until the first switch, and destroy notifications come on the last switch rather than when memory is alloc/dealloc. Will that work for you? If so, we can drop these extra observers.

It seems that other contributors agreed without knowing the other way :)
So, what @trowski did does not seem to make any sense.

You also said:

Of course this is just my personal subjective feeling, It ok if you prefer more new specific APIs :)

There doesn't seem to be a problem.

Yeah, I just want to make you happy.

[twose]

If possible, I would like to ask you to explain how more APIs bring completeness and flexibility, I can't find the answer in the content you quoted. What @trowski said also shows that these things may be redundant.
It makes us have to repeat call these redundant notification APIs in the code, even one of the most popular PHP debug extensions does not need to use it at all.
Of course, you can also contact these extension maintainers to require them to use these APIs instead of check status, to force me to notify them too.
Otherwise, it is difficult for me to recognize their value. It's more like a new historical burden for me.

[krakjoe]

If possible, I would like to ask you to explain how more APIs bring completeness and flexibility

Not every observer will be interested in having their callback executed for every switch. It so happens that the use cases we have in front of us today are interested in every switch. Having a "one callback to rule them all" approach in an API that is supposed to have minimal impact on performance doesn't make sense.

We are designing an API, not catering to specific use cases although specific use cases can guide us.

[twose]

If possible, I would like to ask you to explain how more APIs bring completeness and flexibility

Not every observer will be interested in having their callback executed for every switch. It so happens that the use cases we have in front of us today are interested in every switch. Having a "one callback to rule them all" approach in an API that is supposed to have minimal impact on performance doesn't make sense.

We are designing an API, not catering to specific use cases although specific use cases can guide us.

Thank you for your reply, but I am still confused.

If a use case does not care about the switch, only care about creation and destruction, what is the point of it doing this? The core of fibers is switching, If we drop the switch, it is nothing more than allocating memory and freeing memory, observers didn't even know whether the fiber has ever run.
So how do you prove that you are not making needless considerations for a non-existent use case?

Almost all observers will affect the performance of the application, and they usually work during debugging and are closed in the production environment, If we skip events we don’t care about by checking the status, it will hardly have an unacceptable performance impact.

But the other way around, when we don’t use debug tools (when no one cares about these events), fibers need to trigger more notifications. Why don't you think more about this situation?

At the very least, if your consideration is correct, so should you consider splitting resume and suspend into two notifications?

So, I can't agree with you here.

[krakjoe]

There are two things going on here, the design of fibers and the design of the observer API. Of course, central to fibers is switching, but central to observers is the ability to observe discreet points in execution with a minimal overhead.

Your assertion that observers are only or mostly used at debug time is wrong: Observers must be suitable for observing production environments, that is how they are deployed today in APM applications.

At the very least, if your consideration is correct, so should you consider splitting resume and suspend into two notifications?

Everything is a compromise, the original changes seemed to add a little bit of the kind of flexibility we desire in the observer API without complicating fibers too much.

[twose]

There are two things going on here, the design of fibers and the design of the observer API. Of course, central to fibers is switching, but central to observers is the ability to observe discreet points in execution with minimal overhead.

I don't think you answered my questions about this part...

If a use case does not care about the switch, only care about creation and destruction, what is the point of it doing this? The core of fibers is switching, If we drop the switch, it is nothing more than allocating memory and freeing memory, observers didn't even know whether the fiber has ever run.
So how do you prove that you are not making needless considerations for a non-existent use case?

And I can not see minimal overhead, I just see more overhead, 1 callback -> 3 callbacks.

Your assertion that observers are only or mostly used at debug time is wrong: Observers must be suitable for observing production environments, that is how they are deployed today in APM applications.

I know that, I've said something about it (and I use usually instead of always, am I wrong? : (
Almost all APM applications have a non-negligible impact on performance, but the observer API calls are almost negligible here. It makes me feel that you don’t seem to have done any assessments, you are just thinking...

@morrisonlevi @SammyK @trowski @bwoebi
I also want to hear what other people think now. And please forgive me if I disturbed you all.

[morrisonlevi]

For the most immediate need I have for observing fibers, I don't care about switch events at all. For this use-case, having to inspect all "switch" events to check a status of init/destroy would add more overhead than subscribing only to init and destroy.

If you want to handle everything inside of switch event, then your observer can do that. It seems that fiber observers only pay for what they are interested in, which is one of the core principles.

Does that make sense, @twose ? Unless I've missed something, the design seems to be working as intended.

[twose]

@morrisonlevi Thanks for your reply!

I suddenly understood why we can't understand each other...
If I understand you correctly, your program will bind all the context to the zend_fiber_context structure. And your program is strongly bound to PHP and will not contain any affected third-party libraries that using global vars. So any switching will not affect your program, you do not care about it.

And the key is the name of init and destroy. zend_observer_fiber_init_notify() was called at zend_fiber_init_context(), but fibers does not say that start must be called immediately after init.

In our implementation, init_context is called in constructer, so if there are any resource allocation errors, it will be thrown in the __construct() instead of start(). But PHP fiber binds init_context() to Fiber::start() implicitly. And fiber was designed as a final class, so I can't understand the meaning of separating constructor from start() method for now @trowski .

So the event name should not be init here, init (zend_fiber_init_context()) just represents a memory allocation. We should call it as start event instead of init.

However, we need something like zend_observer_fiber_start_notify() and zend_observer_fiber_end/finished_notify(), but it seems that we can't modify it any more, so I think we should add comments for these two notify, what do you think? @krakjoe :)

[twose]

For the most immediate need I have for observing fibers, I don't care about switch events at all. For this use-case, having to inspect all "switch" events to check a status of init/destroy would add more overhead than subscribing only to init and destroy.

@morrisonlevi

Let me talk something about the other part...

These two observers do make sense to you, but I also have reasons not to like them so much.

Fiber observers are not like the fcall observer, even for an IO-intensive application that has thousands of long connections, its switching overhead is not significant throughout the application.

PHP fiber has a heavier context switch than C fiber, it is many times slower than pure C fiber switching. And the fiber switch is always accompanied by IO operations or syscalls, syscall is so expensive that it makes all other optimizations seem trivial.

And I am not willing to say that fiber can also be used in mathematical calculations such as Fibonacci, which is a very common example. For mathematical programs, fiber's switching performance is so poor that it performs much worse than conventional programming modes, it's like a toy in this context.

So I think you probably don't have to worry too much about the extra cost of switch event callbacks, but anyway, you deserve more, thank you again for your reply :)