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

Low performance of TextResponse and another related responses #58

Open
codercms opened this issue Nov 30, 2020 · 11 comments
Open

Low performance of TextResponse and another related responses #58

codercms opened this issue Nov 30, 2020 · 11 comments

Comments

@codercms
Copy link

Feature Request

Q A
New Feature no
RFC no
BC Break no

Summary

Currently TextResponse always allocating new buffer for string output - https://github.com/laminas/laminas-diactoros/blob/2.6.x/src/Response/TextResponse.php#L75
This approach working really slow, my suggestion is to implement pure RAM string stream.
Here is what I mean - https://github.com/makise-co/framework/blob/master/src/Http/FakeStream.php (this approach is ~30% more performant)

@lcobucci
Copy link
Member

@codercms are you always experiencing that low performance or it depends on the text length? Does anything change if /maxmemory:NN gets added to php://temp? The filesystem should only be used when the text is big (default limit is 2Mb).

@Ocramius
Copy link
Member

my suggestion is to implement pure RAM string stream.

Overall agree, php://memory could suffice here, but we also are not sure if the provided string|StreamInterface will fit in memory, hence why a more conservative php://temp was used here.

I suggest sending a patch, and then we discuss the implications there: it is also acceptable to tell end-users that TextResponse is not intended for large payloads (which is in fact the original design anyway)

@codercms
Copy link
Author

@lcobucci there is two cases:

  1. I'm experiencing low performance because TextResponse doing excess allocation of PHP native stream (allocation time + string copying time)
  2. With large text length performance difference is even more

Patched TextReponse:

$size = 10 * 1024 * 1024;
$body = new Stream("php://temp/maxmemory:{$size}", 'wb+');

It is still slower than approach without PHP native stream allocation.
320 RPS without native PHP stream
260 RPS with native PHP stream

@codercms
Copy link
Author

@Ocramius I guess this patch will not affect end users, because php native stream is only allocated when string was passed to the constructor - this means that the memory for response is already allocated.

@Ocramius
Copy link
Member

Looking back at this, I think that for text, we can use php://memory, and let consumers with large text/plain payloads implement their own streamed response perhaps?

Unsure if this should be considered a BC break.

@Ocramius
Copy link
Member

Similar for html, which is the more common response object.

@codercms
Copy link
Author

@Ocramius I think there is one more approach (in two different ways):

  1. Create a “fake” stream object that is just simulating stream behavior (under the hood its just a string wrapper which implements StreamInterface) - end user will have to create stream object
  2. Create a true plain string response which will create a “fake” stream object under the hood - no additional actions for end user

@Ocramius
Copy link
Member

I didn't really understand the difference between those suggested approaches 🤔

@codercms
Copy link
Author

@Ocramius actually its only one approach which can be implemented in two manners

@Ocramius
Copy link
Member

But basically, if I understand it correctly, avoiding usage of a stream at all, and having a string variable instead (since html/text responses are generally well within the memory limits)

@codercms
Copy link
Author

@Ocramius yes it is. The performance problem occurs when the Response object allocates stream for string (I mean PHP stream), but actually there is no need to allocate memory one more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants