Skip to content

Conversation

@WyriHaximus
Copy link
Member

This middleware runner is faster and uses less RAM the more middleware added because it doesn't creates new instances of it self while iterating over all the middleware.

@jsor
Copy link
Member

jsor commented Oct 11, 2017

Not strictly related to this PR, but the runner could need a test that it can be run multiple times without internally modifying the middleware stack. This might be avoiding regressions when refactoring.

$runner = new MiddlewareRunner([
    $this->expectCallableExactly(2),
    $this->expectCallableExactly(2)
]);

$runner(new ServerRequest('GET', 'https://example.com/'));
$runner(new ServerRequest('GET', 'https://example.com/'));

@WyriHaximus
Copy link
Member Author

@jsor could you PR that, so we can add that test before merging this PR?

@jsor
Copy link
Member

jsor commented Oct 11, 2017

@WyriHaximus damn, i failed trying to offload work 😄 Here it is: #237

@WyriHaximus
Copy link
Member Author

@jsor You can try 😝 . Cheers 👍

@WyriHaximus WyriHaximus force-pushed the improved-middleware-runner branch from 0b5f9a6 to a5c32a8 Compare November 15, 2017 16:33
@WyriHaximus
Copy link
Member Author

Ping @clue @jsor rebased from master and fixed the middleware runner to pass all tests

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

The test is currently failing, otherwise LGTM 👍

Can you give some numbers on how this is "improved"?

@WyriHaximus
Copy link
Member Author

@clue yeah only 5.3 test is failing at the moment. Current master is more over 3 times slower than this PR. Also only only has all the middlewares in memory once and no extra references to them like the current master has saving memory. (Depending on how many middlewares you have and how many requests you're handling at that moment that adds up.)

@WyriHaximus WyriHaximus force-pushed the improved-middleware-runner branch from a5c32a8 to 298cdcf Compare November 15, 2017 21:22
@WyriHaximus
Copy link
Member Author

@jsor @clue fixed PHP 5.3 support and it seems that doubled the performance

return new Promise\Promise(function ($resolve, $reject) use ($middleware, $request, $func, &$cancel, &$position) {
$position++;
try {
$cancel = $middleware(
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, this should be $response insteand of $cancel.

@WyriHaximus WyriHaximus force-pushed the improved-middleware-runner branch from 298cdcf to 260463f Compare November 16, 2017 16:34
@WyriHaximus
Copy link
Member Author

@jsor Good point, changed it 👍

@WyriHaximus WyriHaximus force-pushed the improved-middleware-runner branch 5 times, most recently from e3ed6ea to 110cef9 Compare November 17, 2017 13:47
…ead of creating a new instance of itself for each middleware ran
@WyriHaximus WyriHaximus force-pushed the improved-middleware-runner branch from 110cef9 to 70b7a48 Compare November 17, 2017 13:47
@clue clue changed the title Improved middleware runner Improved middleware runner performance Nov 17, 2017
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Nice, let's get this in! 📈 🎉

@WyriHaximus WyriHaximus merged commit ffe6126 into reactphp:master Nov 17, 2017
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