Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sleep(0) is valid #6955

Merged
merged 1 commit into from Nov 22, 2021
Merged

sleep(0) is valid #6955

merged 1 commit into from Nov 22, 2021

Conversation

BenMorel
Copy link
Contributor

@weirdan
Copy link
Collaborator

weirdan commented Nov 22, 2021

What's the use-case though? Is it to yield CPU to other processes?

@BenMorel
Copy link
Contributor Author

Our use case is a CLI that accepts an optional startup delay, and we do accept 0 seconds.

Anyway, I think Psalm should align with PHP's behaviour, that complains only for < 0?

@orklah
Copy link
Collaborator

orklah commented Nov 22, 2021

We do have this issue too in our codebase. Our default sleep duration is 0 but can be changed. It feels cheap to make a special condition to avoid calling a sleep instruction that will do nothing

@weirdan
Copy link
Collaborator

weirdan commented Nov 22, 2021

There's a difference between not calling sleep() at all and calling it with 0 argument, most noticeable when the system is under load. E.g. running paratest on Psalm test suite with Xdebug enabled (to generate load) and the following script in parallel:

<?php

$s = microtime(true);
$t = $argv[1];
if ($t >= 0) {
    sleep($argv[1]);
}

var_dump('elapsed', number_format(microtime(true) - $s, 6));

I get:

$ time php qq.php -1                                                                                     
string(7) "elapsed"
string(8) "0.000018"
user=0.14s system=0.05s cpu=19% total=0.980

$ time php qq.php 0
string(7) "elapsed"
string(8) "0.014775"
user=0.16s system=0.03s cpu=20% total=0.929

(LA ~10).

So 0 is not totally useless, as it does produce measurable effect.

@weirdan weirdan added the release:fix The PR will be included in 'Fixes' section of the release notes label Nov 22, 2021
@weirdan weirdan merged commit cd8ba5c into vimeo:master Nov 22, 2021
@weirdan
Copy link
Collaborator

weirdan commented Nov 22, 2021

Thanks!

@orklah
Copy link
Collaborator

orklah commented Nov 22, 2021

Is it any different than another "no-action" function call? Let's say strtolower on an empty string?

@weirdan
Copy link
Collaborator

weirdan commented Nov 22, 2021

It is.

Quoting nanosleep() man page (sleep() is using nanosleep() under the hood):

nanosleep() suspends the execution of the calling thread until either at least the time specified in *req has elapsed, or [...]
Furthermore, after the sleep completes, there may still be a delay before the CPU becomes free to once again execute the calling thread.

The net effect of that is that the longer the CPU runqueue, the later your process will be waked up, on average. sleep(0) still suspends your process, and it then must wait its turn to use the CPU again.

@orklah
Copy link
Collaborator

orklah commented Nov 22, 2021

Good to know, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants