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

Always store Response in memory #227

Closed
wants to merge 2 commits into from
Closed

Always store Response in memory #227

wants to merge 2 commits into from

Conversation

mvorisek
Copy link

@mvorisek mvorisek commented Apr 7, 2023

Response buffer should always stay in memory for two reasons:

  • a) performance
  • b) permission and/or read only disk

man https://www.php.net/manual/en/wrappers.php.php

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Apr 8, 2023

Can you back both claims with some data? I never heard about a system without a tmp directory so claim b) looks non-existant to me, and claim a) would need a scenario where this matters, and I've yet to see one. Also, this ignores the memory consumption this change would incur.

👎 unless proven otherwise.

@mvorisek
Copy link
Author

mvorisek commented Apr 8, 2023

The memory consumption should be ok as the code is reached from a string already present in memory. If the source data were not a string (Stream or resource), the code is not reached.

a) performance

This is how I discovered this problem, we have been observing high SSD usage, as our API responses are often above 2MB php://temp treshold.

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Apr 8, 2023

Ah, makes sense now thanks for the insights.
I'm wondering if PHP copies the string or if it just references it when writing to the stream. Could you have a look by checking the memory consumption before/after?

@mvorisek
Copy link
Author

mvorisek commented Apr 8, 2023

The data are duplicated.

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Apr 9, 2023

I think it's possible to create a stream wrapper to prevent the duplication. I would take https://www.php.net/manual/en/stream.streamwrapper.example-1.php as a start and instead of relying on globals, I would initialize the buffer via a custom stream context.

But I'm not saying this to block this PR. This could be contributed later.

@nicolas-grekas
Copy link
Collaborator

Idea: add a stream context option to the php://memory stream wrapper to initialize the buffer on fopen and prevent the duplication natively. Could you open an issue on php-src so that the idea can be considered?

@mvorisek
Copy link
Author

mvorisek commented Apr 9, 2023

I do not think the data are internally stored in zval. I would say the custom stream wrapper can be a solution to the duplication, but I am not fully sure if it is worth it, as normally the string is build, released, and thus no deduplication is needed.

@GrahamCampbell
Copy link
Contributor

This is quite a breaking change, and will break things for people using low-memory environments, who need to move things to disk.

@mvorisek
Copy link
Author

mvorisek commented Apr 9, 2023

Yes, but as this change affects only input already stored in memory/string, the memory consumption should not be a major BC-break.

@GrahamCampbell
Copy link
Contributor

Fair.

@nicolas-grekas
Copy link
Collaborator

I propose #230 instead.

@mvorisek
Copy link
Author

closing in favor of #230

@mvorisek mvorisek closed this Apr 11, 2023
@mvorisek mvorisek deleted the patch-1 branch April 11, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants