Skip to content
This repository was archived by the owner on Jan 17, 2022. It is now read-only.

WIP: feat(coroutines): ability to use coroutines in Symofny#566

Open
Rastusik wants to merge 1 commit into
k911:developfrom
pixelfederation:coroutines
Open

WIP: feat(coroutines): ability to use coroutines in Symofny#566
Rastusik wants to merge 1 commit into
k911:developfrom
pixelfederation:coroutines

Conversation

@Rastusik

@Rastusik Rastusik commented Dec 12, 2021

Copy link
Copy Markdown
Contributor

Hi @k911 ! :)
This PR is an POC WIP implementation of #561 . Would you please take a look at? What do you think?

TLDR: I was able to use Swoole coroutine hooks (https://www.swoole.co.uk/docs/modules/swoole-runtime-flags) in Symfony, which effectively means, that it is now possible to concurrently execute multiple requests in a Symfony application. The base idea is described in the issue.

Basically what is being done is, that the Symfony container is overridden to create multiple instances of stateful services (which would be a problem for concurrent request processing, if there would be only one instance of them - race conditions would occur). These multiple instances behave as on instance from the POV of the SF container and the app, because they are all hidden behind one service instance (a proxy, which forwards method calls to the right service instance according the current coroutine identifier/context)

When the coroutine hooks are enabled, one request processing will wait on each IO operation, which will make way for a different request to be able to be processed. This way, each IO call is effectively changed to an async/await call in the background of the runtime. It's also possible to use Doctrine with this, the code should be platform independent and now it is possible to use Doctrine in concurrent code.

As a bonus, it is also possible to use the Swoole coroutines, but not by calling Co::go(...), but by calling CoWrapper::go(...), which wraps the original Swoole function and runs some garbage collection after the coroutine is finished.

There still are some quirks to be figured out, e.g. the app kernel has to override the __clone() method of Symfony\Component\HttpKernel\Kernel, because Kernel is a stateful service as well... But it works good for now. The other quirk is that this code doesn't run with final stateful services, but this problem is solvable as well.

It is also interesting to observe some benchmarks. I got these numbers using debug:off, WORKER_COUNT:1 REACTOR_COUNT:1 (env:coop_scheduling with Sqlite database with coroutines turned on (I used APCu as cache backend, not php arrays, as committed):

❯ wrk -d 10 -t 4 -c 10 http://localhost:9501/doctrine
Running 10s test @ http://localhost:9501/doctrine
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    22.16ms    7.10ms 117.02ms   94.92%
    Req/Sec    91.73     17.25   121.00     76.75%
  3664 requests in 10.04s, 4.30MB read
Requests/sec:    365.00
Transfer/sec:    438.42KB

And with coroutines turned off:

❯ wrk -d 10 -t 4 -c 10 http://localhost:9501/doctrine
Running 10s test @ http://localhost:9501/doctrine
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    24.43ms    7.60ms 123.23ms   95.69%
    Req/Sec    83.03     14.69   121.00     68.25%
  3321 requests in 10.03s, 3.90MB read
Requests/sec:    331.05
Transfer/sec:    397.64KB

That's just a slight increase. Not sure how the Sqlite driver works but it seems that it is doing quite a lot in memory, so there is less IO and worse results.

But when testing with MySql as a backend, the results are much more interesting for one web worker process with coroutines:

❯ wrk -d 10 -t 4 -c 10 http://localhost:9501/doctrine
Running 10s test @ http://localhost:9501/doctrine
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    10.48ms    4.06ms  52.33ms   85.85%
    Req/Sec   193.63     43.76   303.00     66.25%
  7725 requests in 10.01s, 9.06MB read
Requests/sec:    771.43
Transfer/sec:      0.90MB

With coroutines disabled, one web worker process was able to handle only half of the requests:

❯ wrk -d 10 -t 4 -c 10 http://localhost:9501/doctrine
Running 10s test @ http://localhost:9501/doctrine
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    22.32ms    4.60ms  58.04ms   82.86%
    Req/Sec    89.69     14.50   121.00     72.25%
  3584 requests in 10.02s, 4.20MB read
Requests/sec:    357.51
Transfer/sec:    429.43KB

This should be just the first step, I would like to make something similar for the Swoole task workers.

@k911 what would you say? Will this be possible to merge? Feel free to suggest any changes or improvements.

@k911

k911 commented Dec 12, 2021

Copy link
Copy Markdown
Owner

Hi, as I don't really have time for this project (as you can see I'm not developing anything new). I can merge it once it passes CI fully so I can release it afterwards. The only requirements are the CI green, and that changes are backward compatible.

$this->coWrapper->defer();
$httpFoundationRequest = $this->requestFactory->make($request);
$this->processorInjector->injectProcessor($httpFoundationRequest, $response);
$kernel = $this->getKernel();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rename "$kernel" which has the same name as the field declared at line 21.

private static function getInstance(): self
{
if (null === self::$instance) {
throw new RuntimeException('CoWrapper was not initialised.');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Define and throw a dedicated exception instead of using a generic one.

/**
* Generator for proxies with service pool.
*/
class AccessInterceptorServicePoolGenerator implements ProxyGeneratorInterface

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The class AccessInterceptorServicePoolGenerator has a coupling between objects value of 14. Consider to reduce the number of dependencies under 13.

/**
* Utility to service pool method interceptor.
*/
class InterceptorGenerator

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Possible parse error: class missing opening or closing brace

/**
* Utility to service pool method interceptor.
*/
class InterceptorGenerator

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Possible parse error: class missing opening or closing brace

@qlty-cloud-legacy

Copy link
Copy Markdown

Code Climate has analyzed commit 929969a and detected 13 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Style 6
Clarity 5
Bug Risk 1

The test coverage on the diff in this pull request is 0.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 16.8% (-69.2% change).

View more on Code Climate.

@codecov

codecov Bot commented Dec 12, 2021

Copy link
Copy Markdown

Codecov Report

Merging #566 (929969a) into develop (da33c81) will decrease coverage by 70.26%.
The diff coverage is 0.00%.

❗ Current head 929969a differs from pull request most recent head f0e0773. Consider uploading reports for the commit f0e0773 to get more accurate results
Impacted file tree graph

@@              Coverage Diff               @@
##             develop     #566       +/-   ##
==============================================
- Coverage      86.03%   15.77%   -70.27%     
- Complexity       654      737       +83     
==============================================
  Files             96      115       +19     
  Lines           2070     2364      +294     
==============================================
- Hits            1781      373     -1408     
- Misses           289     1991     +1702     
Impacted Files Coverage Δ
src/Bridge/Doctrine/DoctrineProcessor.php 0.00% <0.00%> (ø)
src/Bridge/Doctrine/ORM/EntityManagerResetter.php 0.00% <0.00%> (ø)
...dge/Doctrine/ORM/EntityManagerStabilityChecker.php 0.00% <0.00%> (ø)
...ection/CompilerPass/StatefulServices/Proxifier.php 0.00% <0.00%> (ø)
...ncyInjection/CompilerPass/StatefulServicesPass.php 0.00% <0.00%> (ø)
...mfony/Bundle/DependencyInjection/Configuration.php 0.00% <0.00%> (-94.40%) ⬇️
...ony/Bundle/DependencyInjection/SwooleExtension.php 0.00% <0.00%> (-76.32%) ⬇️
src/Bridge/Symfony/Bundle/SwooleBundle.php 0.00% <0.00%> (-100.00%) ⬇️
src/Bridge/Symfony/Container/CoWrapper.php 0.00% <0.00%> (ø)
...Generation/AccessInterceptorServicePoolFactory.php 0.00% <0.00%> (ø)
... and 73 more

@Rastusik

Copy link
Copy Markdown
Contributor Author

great, I'll look into it! :)

@Rastusik Rastusik force-pushed the coroutines branch 2 times, most recently from 1d0a531 to 92daab6 Compare December 30, 2021 15:24
@Rastusik Rastusik force-pushed the coroutines branch 8 times, most recently from a3f7b7d to f04782e Compare January 9, 2022 18:11
@Rastusik

Rastusik commented Jan 9, 2022

Copy link
Copy Markdown
Contributor Author

@k911 the checks need to be approved to run? I'm not sure which checks you'd like me to fix, if they can't run :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants