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

Remove internal Queue leftovers #238

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Conversation

clue
Copy link
Member

@clue clue commented Oct 6, 2022

This changeset removes any internal Queue leftovers. This does not change outside behavior of this project and only cleans up unneeded code, so this does not break BC.

This effectively reverts the remainder of the iterative logic introduced in #28/#82/#86. This logic has turned out to cause a number of problems in the past, so the behavior has been reverted as part of #229 already. This means that the iterative approach originally planned for Promise v3 is now reverted completely and the logic is in line with how Promise v2 and v1 behave. The test suite confirms my changes do not break any of the previous assumptions, so this should be safe to apply.

On top of this, this avoids a number of function calls and shows a significant performance improvement in synthetic benchmarks. For example, the following benchmark improved from 16.4s to 11.6s 3.1s on my machine (best of 5 each):

$n = 10_000_000;
for ($i = 0; $i < $n; ++$i) {
    React\Promise\resolve($i)->then(function () { });
}

I still agree that an iterative approach sounds tempting, but at this point it's unclear what benefit this would actually bring and if it's worth breaking any of the existing behavior. I would suggest we should look into this iterative logic and its consequences again in a follow-up PR some time in the future if we see a need for this. Until we can come up with more specific test cases that highlight now this iterative logic is needed, I would argue that merging this as is to remove any unneeded code and to reduce our complexity seems reasonable.

@clue clue added this to the v3.0.0 milestone Oct 6, 2022
@clue clue requested a review from WyriHaximus October 6, 2022 11:07
@clue
Copy link
Member Author

clue commented Oct 6, 2022

Updated to use resolve() instead of new Promise(…) for wrapping the results as done prior to #28. Again this does not break BC, but significantly improves the benchmark results from 16.4 to 11.6s to 3.1s. The benchmark for 3.x is now pretty much in line with 2.x again.

@WyriHaximus WyriHaximus merged commit d76ba7b into reactphp:3.x Oct 10, 2022
@clue clue deleted the dequeue branch October 11, 2022 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants