support VM_RETURN vm_interrupt

[taoso]

I wrote a RFC for PR #2886. After published in the php-internal list, @dstogov pointed out that

It should be possible do similar things using EG(vm_interrupt) and zend_interrupt_function() callback (introduced in php-7.1)
ext/pcntl implements asynchronous signal handling using this.

I read the code of EG(vm_interrupt), almost every thing I proposed in PR #2886 could be implemented by the EG(vm_interrupt). However, the EG(vm_interrupt) can only pause-and-continue, but I propose to introduce a pause-and-return API.

So I propose to introduce another to make the minimum changes to php-src base.

And the RFC has also been updated.

[krakjoe]

@dstogov @nikic @bwoebi we're okay to merge this, right ? an RFC looks like a waste of time to me, it's a minor implementation detail ...

[taoso]

@krakjoe could this patch be merged into the PHP-7.1 branch? So that all version above PHP-7.1 could support this feature.

[krakjoe]

Can only target master.

[jhdxr]

Can you make a constant? Magic number is not a good idea IMHO

[taoso]

Could you @dstogov @nikic @bwoebi make your comment? Thank you.

[dstogov]

I think, this change should be considered together with Fiber implementation (please add a link into RFC), because we don't need to introduce unused VM features.

Personally, I think that pceudo-cuncurency primitives (like Fibers), should be implemented in stack-less way (without C stack usage, and invocation of new copy of interpreter loop).

Looking into https://github.com/php/php-src/compare/master...fiberphp:fiber-ext?expand=1 I see two ways to switch context:

Fiber::resume() - actually, starts new instance of interpreter
Fiber:: yield() - returns into previous instance of interpreter

In case you write some kind of scheduler in PHP using Fiber::resume(), you are going to overflow C stack at some point...

I propose, to try switching context in the single interpreter loop instance (without terminating it in VM_INTERRUPT), and then we don't need this patch at all.

However, the patch is small and won't make any harm.

@nikic?

[taoso]

Hi @dstogov

As @nikic pointed out, my Fiber implementation can cause C stack overflow when you call the Fiber::resume during a internal callback like

<?php
array_map(function () {
    Fiber::yield();
}, $arr);

An easy way to avoid this is avoiding call the Fiber::yield during the internal call's callback.

I will try my best to work on your proposed solution(It is awesome) of switching context in the single interpreter loop instance. However, the current implementation still be useful.

[dstogov]

I think, when you implement context switching in vm_interrupt() callback, you won't need to return from interrupt anymore. And the VM stack part of the patch is not a big problem and we may add it in master, if you make fibers work.

The good thing, that fibers will also able to work on unmodified PHP-7.1 and 7.2.

[taoso]

@dstogov Even the context switching can be implemented by the pause_and_continue vm_interrupt method, it does not make any more sense at all.

• For one thing, this way is to much complicated. We have to prepare a pceudo oparray, to trigger a function call.
• For the other, this way does not resolve the c stack overflow problem. Because every time you call the zend_call_function api, you will start a new instance of interpreter.

because we don't need to introduce unused VM features.

All the features defined in this patch seems useless for zend_vm. They just open some inner ability of Zend VM to the 3rd extension and make no harm. I think they allow the extension to do things more interesting. And Merge them into the main branch would be a good convenient.

The good thing, that fibers will also able to work on unmodified PHP-7.1 and 7.2.

A Fiber with a stack of the default size(more than 256k) is also useless.

Thank you.

[bwoebi]

Quoting Dmitry:

I propose, to try switching context in the single interpreter loop instance

So, he suggests to use APIs like getcontext(), setcontext() etc. in order to properly save internal calls and continue on another stack. If you do that, there's indeed no need for this patch.

[dstogov]

@bwoebi, actually I told only about VM context that is kept in EC(current_execute_data).

[dstogov]

@lvht could you please create a github repo with Fiber extension only. I'll try to play with it a bit.

[taoso]

@dstogov Please see https://github.com/fiberphp/fiber-ext

[dstogov]

@lvht @nikic @bwoebi See https://github.com/dstogov/fiber-ext/tree/stackless
I've re-factored the extension to make resume and yield transfer control without C stack usage.

[taoso]

@dstogov You work is awesome and inspired. I will make more test. And I am wondering whether the zend patch you offered could be merged into the main line.

Thank you.

[dstogov]

@lvht Changes like this may be merged only into master. Actually, your extension really need only the change in zend_vm_stack_extend(). And especially this change is not completely keep the old behavior. Previously, a large stack segment (greater than ZEND_VM_STACK_FREE_PAGE_SIZE) didn't affect the following segment allocations. With the change, once we allocate a large segment, all the following segments are also going to be large. Anyway, we may find a solution for master.

[taoso]

@dstogov What about make the _zend_vm_stack store a new field frame_size, and set it in the zend_vm_stack_init_ex, and use it int the zend_vm_stack_extend?

[dstogov]

yeah, this will work.

[taoso]

Hi @dstogov , I have updated this patch.

[taoso]

Ping @dstogov

[taoso]

Please offer your @dstogov @nikic @bwoebi @krakjoe comment. Thank you.

[dstogov]

@lhvt, hi. sorry for delay. I don't see a big problem committing this patch into master with minimal changes. I would just move frame_size into executors_global, and check if exported functions don't affect performance.

BTW I think, fibers or coroutines should be implemented directly in Zend Engine. Do you like to propose this? Write RFC that defines API and show cool use case, lead discussion... I may help with implementation details.

@nikic @bwoebi @krakjoe?

[taoso]

@dstogov

Do you like to propose this? Write RFC that defines API and show cool use case, lead discussion...

Yes, I do. I will update the RFC later.

[uasan]

Yes, I do. I will update the RFC later.

I would like to use Fiber, but convenient language syntax, the need new PHP keywords async/await

$fiber = new Fiber(function(){}); // uncomfortable
$fiber = async function(){};     // ok

$result = $fiber->resume(); // uncomfortable
$result = await $fiber();  // ok

Thanks.

[taoso]

closed by 6780c74

[dstogov]

I've committed a similar patch into master 6780c74 and updated the extension code accordingly https://github.com/dstogov/fiber-ext/tree/stackless