Skip to content

Conversation

@twose
Copy link
Member

@twose twose commented Dec 25, 2018

6780c74

@dstogov
I am confused when I read the source code, even if zend_vm_stack_init does not provide parameters to set it, we can write our own init function, but when zend_vm_stack_extend and size > EG(vm_stack_page_size), it still use ZEND_VM_STACK_PAGE_SIZE (256k?), if it's a little big when page_size is 4k or 8k for a coroutine?
emmm... and I do hope that you can provide more powerful APIs for coroutine...
Thanks!

@dstogov
Copy link
Member

dstogov commented Dec 25, 2018

May be I misunderstood this without the context, but I think the patch is wrong.
I assume, this is for swoole coroutines (or something like this)?

  1. Don't use zend_vm_stack_init() for coroutine stack creation (use your own function).
  2. When you switch stack, you also should change EG(vm_stack_page_size), and then zend_vm_stack_extend() will use it out of the box.

@twose
Copy link
Member Author

twose commented Dec 26, 2018

Sorry, I was confused last night and I thought wrong for something, thank you for your prompt answer.
You are right, I know that I should use my own function, I just noticed that we have EG(vm_stack_page_size) in PHP73, and then I replace ZEND_VM_STACK_PAGE_SIZE to EG(vm_stack_page_size).
In ZendVM, they are no difference, but when we can change it, it's more in line with its significance?
Then whether ZEND_VM_STACK_PAGE_ALIGNED_SIZE should use EG(vm_stack_page_size)?
when I use:

<?php
$params = '';
for ($n = 0; $n < 256; $n++) {
    $params .= "\$p{$n} = null;\n";
}
$gen = <<<PHP
<?php
go(function () {
{$params}
});
PHP;
file_put_contents(__DIR__ . '/extend.php', $gen);

In coroutine EG(vm_stack_page_size) = 8192, 256 zval + some other necessary memory are bigger than it (actually 8272), I expect the next new page size is 16384, but I got 262144. Maybe this is not a problem? but I just want to know, thanks.

@dstogov
Copy link
Member

dstogov commented Dec 26, 2018

you are right. This has to be fixed. I'll think, what to do.

@dstogov
Copy link
Member

dstogov commented Dec 26, 2018

Please check if this patch works for you
https://gist.github.com/dstogov/cadc9c28f42483b9d44e427321f54176
Actually, your patch was right too.

@twose
Copy link
Member Author

twose commented Dec 26, 2018

@dstogov Yes! This is what I want! Thanks! your ALIGNED macro is better for using.

@twose twose force-pushed the vm_stack_page_size branch from e5fe419 to 388c31e Compare December 26, 2018 08:43
@twose
Copy link
Member Author

twose commented Dec 26, 2018

😵, tab and space mixed in some places, maybe petk can slove these... it looks good now.

@dstogov
Copy link
Member

dstogov commented Dec 26, 2018

I've merged the patch into master and PHP-7.3

@twose
Copy link
Member Author

twose commented Dec 26, 2018

👍OK, thank you very much!

@twose twose closed this Dec 26, 2018
@twose
Copy link
Member Author

twose commented Dec 26, 2018

@dstogov
is it necessary to add an API to set php_stack_page_size in swoole? In swoole, the default page size is 8K, In some cases, if small stack page size will lead to stack extend frequently?
Need your valuable advice, thanks!

@twose twose deleted the vm_stack_page_size branch December 26, 2018 11:02
@dstogov
Copy link
Member

dstogov commented Dec 26, 2018

I would add debug counters or logging into zend_vm_stack_extend(), then run few apps and made decision based on collected data.

@twose
Copy link
Member Author

twose commented Dec 26, 2018

Thanks, I will have a try too.

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.

2 participants