Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Improve memory efficiency#18932
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Improve memory efficiency#18932
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This avoids concatenating `$message` and `PHP_EOL` (if necessary) as a new value, greatly improving memory efficiency for large `$message`s.
dword-design commentedJun 1, 2016
This is my first pull request, please don't hurt me :) |
| protectedfunctiondoWrite($message,$newline) | ||
| { | ||
| if (false === @fwrite($this->stream,$message.($newline?PHP_EOL :''))) { | ||
| if (false === @fwrite($this->stream,$message) ||($newline&& (false === @fwrite($this->stream,PHP_EOL)))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what about:
if (false === @fwrite($this->stream,$message)) {// should never happenthrownew \RuntimeException('Unable to write output.'); }if ($newline) { @fwrite($this->stream,PHP_EOL); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In this case no Exception would be thrown if something goes wrong with writing PHP_EOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yep, which won't happen if the first fwrite already succedeed IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could be but what speaks against the safe solution?
OskarStark commentedJun 1, 2016
can you show some statistics about that? |
javiereguiluz commentedJun 1, 2016
@Dword123 congrats on your first pull request and thanks for contributing to Symfony! The kind of improvement that you propose here is always tricky for us. This affects the console output stream, which is something that it's not used frequently or on every request. That's why most of the times these improvements don't provide enough gains to be worthy. So let's see what other contributors think about this. |
mpdude commentedJun 1, 2016
We're using symfony/console over athttps://github.com/webfactory/slimdump, a tool that makes SQL dumps to In that case, the If the newline is added to the string before being written, PHP has to allocate additional memory about the size of Asked the other way round: What problem could this change cause? |
nicolas-grekas commentedJun 2, 2016
👍 |
nicolas-grekas commentedJun 3, 2016
Thank you @Dword123. |
This PR was merged into the 2.7 branch.Discussion----------Improve memory efficiency| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This avoids concatenating `$message` and `PHP_EOL` (if necessary) as a new value, greatly improving memory efficiency for large `$message`s.Commits-------c1df9f2 Improve memory efficiency
This avoids concatenating
$messageandPHP_EOL(if necessary) as a new value, greatly improving memory efficiency for large$messages.