Skip to content

Conversation

trowski
Copy link
Member

@trowski trowski commented May 26, 2021

This additional internal fiber API creates and manipulates a Fiber object, allowing any internal function to start, resume, or suspend a fiber. The existing zend_fiber_context API allows custom C-based fiber creation using the bundled switching context, but does not interact with the PHP VM. This API uses zend_fiber and behaves the same as calling Fiber object methods from user code, switching EGs, and triggering the fiber switch observer. In general, the Fiber object methods call these new API methods.

This additional internal fiber API creates and manipulates a Fiber object, allowing any internal function to start, resume, or suspend a fiber. The existing zend_fiber_context API allows custom C-based fiber creation using the bundled switching context, but does not interact with the PHP VM. This API behaves the same as calling Fiber object methods from user code, switching EGs, and triggering the fiber switch observer. In general, the Fiber object methods call these new API methods.
@trowski trowski requested a review from nikic May 26, 2021 04:43
@nikic
Copy link
Member

nikic commented May 26, 2021

cc @twose

@twose
Copy link
Member

twose commented May 26, 2021

I am sorry that I don’t know who else will use these APIs except extensions like Swoole...
At first, zend_fiber_*_context functions do not make sense to me, the custom fibers must also maintain the same memory structure as zend_fiber (or expand its memory structure), otherwise, it can not work with any other fiber APIs. So, zend_fiber_* APIs are enough I think.
To make it be able to work with other extensions like Swoole, I have planned to refactor some of the fiber content (actually I've already done it), but I haven’t had time to split them into multiple PRs for others to review... Maybe I will try to create some PRs before the end of this month. So personally I hope we don’t make more changes for now...

@bwoebi
Copy link
Member

bwoebi commented May 26, 2021

@twose Maybe, if you've already done it - could you just push your work until now onto some branch (no matter if it works or compiles properly), so that we can see the direction it's headed to and potentially give early feedback? That way we can also iterate faster on this internal fiber API as needed. Thanks :-)

@twose
Copy link
Member

twose commented May 26, 2021

@bwoebi
You can take a look at it here: twose@0a43b34#diff-74dfa201ebfaf60fcfe2ddcec6eb6677deb4ee888a933772ab5b222be511f8cf

Ummm, I am not sure if it is appropriate to discuss this in this thread... I can delete it if anyone feels dissatisfied.
The code has done before the current fiber implementation merged, I don’t want to post it now because some changes may make not be comfortable (For example, I changed some names or log formats according to personal preference, It's not necessary).
It can work well, but it is different from the current implementation in some details. There are still some parts of the existing design that I disapprove of, my implementation version does not include them for the time being.

It has the following key differences from the current fiber:

  1. Drop final, it's important, Otherwise, we can't inherit the fiber class for some customization, such as class Swoole\Coroutine extends Fiber {}. (The interesting thing is that the coroutine in Swoole should be called fiber in theory, and the current fiber should be called coroutine...)
  2. I believe that zend_fiber_switch_to and zend_fiber_suspend are the same, zend_fiber_suspend() = zend_fiber_resume(EG(current_fiber)->previous), this part of the code looks confusing, it did the same thing twice, you can take a look at my implementation version, it's more concise.
  3. We only need four statuses, INIT/SUSPENDED/RUNNING/DEAD (same with Lua coroutine), additional information does not need to be expressed in status, they have already be expressed in other fields.
  4. Provided getStatus() is better, we can inherit and implement isXXX() by ourselves (I don’t know if we allow changing this, and it’s just my personal preference, I want to minimize it).
  5. Reflection classes are completely unnecessary.
  6. Introduced main fiber.
  7. Put the fiber object and its context on its own stack. This is the same trick as the zend_vm_stack does. It also works on the main fiber, we can put the main fiber on the main PHP VM stack without malloc.
  8. Change the order of executor_globals members, so that we can switch context through a memcpy instead of multiple assignments. (We don’t need to keep its ABI stable between minor versions, right? We have thought of a lot of optimization ways for this in the extension, but directly changing Zend makes it very simple)
  9. ...(There are a lot of details, I can’t write it all at once here)

For all the design problems, I think we can follow the rules of the multiprocess/multithreaded model:

  1. If one fiber(process) crashes, it should not affect other fibers(processes).
  2. We shouldn't catch exceptions outside of the fiber, It's like we can't catch another thread exception. Regardless of the model to be followed by the fiber design, this information should be passed on in other ways, such as the Channel, maybe that the only thing we can get directly is the exit code.
  3. We need to provide $fiber->getId(), just like process PID/ thread TID, which has many uses, for the most important example, log.
  4. Don't let the main fiber become an empty pointer, or it will become a disaster (I am talking from my experience), and we have to check it everywhere. It's easy to abstract the main stack into a fiber. After we have the scheduler implementation, we just need to swap the scheduler and the main, then the scheduler becomes the root node, and the main fiber is no different from other fibers.
  5. ....(the same as above)

Generally speaking, the code will be more concise. Of course that in order to remove the final, we also add some extra code for it.

Nevertheless, we still need to do a lot of work to make it work with extensions like Swoole/Swow, it's just the beginning...

@trowski
Copy link
Member Author

trowski commented May 26, 2021

@nikic @krakjoe Updated based on feedback, please review again.

@twose A lot of what you're proposing are major changes, some behavior changes would require an RFC. It seems you'll need to rebase your work and start again anyway, as the merged implementation has changed quite a bit since you started. The API I'm proposing here actually aligns better with some of what you wrote, so I think this is a good starting point. Let's merge this PR and we can iterate from there if necessary. As you suggested, each change needs to be a separate PR and discussed – some of the things you mention have been already discussed (such as putting the Fiber object and VM stack on the mmap'd memory segment).

@bwoebi
Copy link
Member

bwoebi commented May 26, 2021

@trowski While some changes could necessitate a RFC if challenged, I think we can do the changes on a consensus based approach. IIRC, you and I also had discussed some things he mentioned, with which you agreed, like making getStatus() an enum of just 4 values.

@trowski
Copy link
Member Author

trowski commented May 26, 2021

@bwoebi Sure, of course not everything proposed (in fact probably not most things) need an RFC.

Now that we have enums, the status could be an enum. I personally prefer the four methods over comparing a constant to a method result – seems less friendly IMO. I don't feel strongly though and this is really minor detail anyway.

Not throwing from start/resume/throw, that probably needs an RFC as it's a major behavior change in the API.

Copy link
Member

@krakjoe krakjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears okay to me.

I've listened to the opinions expressed here, and the concern that we should not merge anything right now. However, this appears to be exactly the kind of iterative improvement that we are expecting, and the suggestions made here seem like the importation of a different implementation, not iterative improvements. While I'm sure that the suggestions made here could be broken down into iterative improvements, I'm not willing to withhold approval on improvements that are ready now.

@twose
Copy link
Member

twose commented May 27, 2021

I've listened to the opinions expressed here, and the concern that we should not merge anything right now. However, this appears to be exactly the kind of iterative improvement that we are expecting, and the suggestions made here seem like the importation of a different implementation, not iterative improvements. While I'm sure that the suggestions made here could be broken down into iterative improvements, I'm not willing to withhold approval on improvements that are ready now.

The code I showed is just another experiment that was done a long time ago. That’s why I didn’t want to show it here in the beginning and I also think that it would be better to break it down into iterative improvements too.
And I do not disapprove of the content of the current patch. Modifying ZEND APIs does not require RFCs, so we can always make more changes before the 8.1 release.

Copy link
Member

@twose twose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's start from here.

@krakjoe krakjoe merged commit 3939c9b into php:master May 27, 2021
@trowski trowski deleted the fiber-api branch May 27, 2021 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants