saloon: [3.x] Feedback: Concurrency & Pools slower than in v2

Thanks so much Sam for the incredible work you do.

This is not of a bug report report but general feedback after playing around with v3.

I understand that the cause may be the design decision to lean more into the building and use of PSR-7 requests as opposed to use Guzzle’s configuration options for v3.

I observed this after a version of the test below started failing upon upgrading to v3. The same benchmark is taking approx 4X more time in v3 than v2.

test('benchmark pooling requests', function (string $phone): void {
    $count = 1_000;
    $expectedTime = 30_000;

    $messages = collect(fake()->sentences($count));

    $responses = collect([]);

    $requests = $messages->map(function (string $message) use ($phone): SmsRequest {
        return SmsRequest::make([
            'message' => $message,
            'to' => $phone,
            'from' => config(key: 'provider.from'),
        ]);
    });

    $connector = MyConnector::make();

    $timeInMilliseconds = Benchmark::measure(
        fn () => $connector->pool(
            requests: $requests,
            concurrency: 10,
            responseHandler: fn (Response $data) => $responses->push($data->dto())
        )->send()->wait()
    );

    expect($timeInMilliseconds)->toBeLessThan($expectedTime); //Less than 30 Seconds to send 1000 messages
})->with('phone-numbers');

I know that the above is a really extreme example but that’s speaks more to how impressive concurrency is. I will take a bit more time to run more tests and try drilling down the root cause of the increased execution times.

Thanks once again and happy coding 🤠

About this issue

Original URL
State: closed
Created 9 months ago
Reactions: 1
Comments: 16

Most upvoted comments

So the issue lies in the SendsRequests.php trait. This is the following code that is causing all the trouble:

Specifically the first and last lines where I define the nested promise and resolve it.

return $promise = new Promise(function () use (&$promise, $request, $mockClient, $sender) {
    $pendingRequest = $this->createPendingRequest($request, $mockClient)->setAsynchronous(true);

    // We need to check if the Pending Request contains a fake response.
    // If it does, then we will create the fake response. Otherwise,
    // we'll send the request.

    // 🚀 ... 🪐  ... 💫

    if ($pendingRequest->hasFakeResponse()) {
        $requestPromise = $this->createFakeResponse($pendingRequest);
    } else {
        $requestPromise = $sender->sendAsync($pendingRequest);
    }

    $requestPromise->then(fn (Response $response) => $pendingRequest->executeResponsePipeline($response));

    // We'll resolve the promise's value with another promise.
    // We will use promise chaining as Guzzle's will fulfill
    // the request promise.

    return $promise->resolve($requestPromise);
});

The code inside isn’t an issue because it will work if I change it to this - and the speed will go back up!

$pendingRequest = $this->createPendingRequest($request, $mockClient)->setAsynchronous(true);

// We need to check if the Pending Request contains a fake response.
// If it does, then we will create the fake response. Otherwise,
// we'll send the request.

// 🚀 ... 🪐  ... 💫

if ($pendingRequest->hasFakeResponse()) {
    $requestPromise = $this->createFakeResponse($pendingRequest);
} else {
    $requestPromise = $sender->sendAsync($pendingRequest);
}

$requestPromise->then(fn (Response $response) => $pendingRequest->executeResponsePipeline($response));

return $requestPromise;

I wrote the wrapper promise because I didn’t want the PendingRequest to be created until the promise was actually being handled. I’d love to keep this functionality - maybe there’s someone out there that knows promises better than I do.

https://github.com/guzzle/promises

Sammyjo20 on Oct 5, 2023

The benchmark to send 1000 messages was taking approx 2 min before the fix which I knew couldn’t be explained by a difference in internet speeds.

SamuelMwangiW on Oct 7, 2023

Thanks so much @Sammyjo20.

I can confirm that the above benchmark is now taking an average of 21 seconds to run which is a significant improvement from when I opened the issue.

Thanks once again for the incredible work you do 👏🏽

SamuelMwangiW on Oct 7, 2023

I think you are on the right track with your thinking on the queue. I’m not at my computer at the moment, but would be curious to play a little bit.

Just by way of clarification, what is the goal with wrapping it in a promise? You are deferring the creation of the pending request until a certain point anyway by having it in a method that is called by the pool send, so all this set up work gets done almost instantly anyway, right?

aidan-casey on Oct 6, 2023