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

[Console] avoid using huge amount of memory when formatting long exception #32736

Merged
merged 1 commit into from Jul 27, 2019

Conversation

paxal
Copy link
Contributor

@paxal paxal commented Jul 25, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT

When formatting exceptions, preg_split('//u') is used to iterate over utf8 characters. When the exception is long, the amount of memory used is huge and can reach memory_limit.

This PR uses a Generator to iterate over the string instead of splitting it, thus reducing the amount of memory.

@nicolas-grekas
Copy link
Member

Can you give us some numbers you measured about this improvement?

@paxal
Copy link
Contributor Author

paxal commented Jul 25, 2019

Please don't judge me ‒ my exception was 1166286 (~1 MB) chars long (very very long doctrine request).

Snippet :

<?php

use Symfony\Component\Console\Application;
use Symfony\Component\Console\Output\NullOutput;

require_once __DIR__.'/vendor/autoload.php';

$start = microtime(true);

$application = new Application();
$longText = str_repeat('Oh my god ! ', 100000);
$application->renderException(new \Exception($longText), new NullOutput());
echo sprintf('%d ms / %d MB', 1000*(microtime(true)-$start), memory_get_peak_usage()/1024**2).PHP_EOL;

Results before (faster, but a lot of memory) : 963 ms / 106 MB
Results after (longer, but less memory) : 5337 ms / 8 MB
With a chunk size of 10 000 (seems good enough to me, I'll send a small patch) : 964 ms / 8 MB

@nicolas-grekas
Copy link
Member

Thank :)
Can you please try this patch instead?
Does it perform better?

--- a/src/Symfony/Component/Console/Application.php
+++ b/src/Symfony/Component/Console/Application.php
@@ -1149,7 +1149,11 @@ class Application implements ResetInterface
         $utf8String = mb_convert_encoding($string, 'utf8', $encoding);
         $lines = [];
         $line = '';
-        foreach (preg_split('//u', $utf8String) as $char) {
+        $offset = 0;
+        while (preg_match('/.{1,1000}/u', $utf8String, $m, 0, $offset)) {
+            $offset += \strlen($m[0]);
+
+            foreach (preg_split('//u', $m[0]) as $char) {
             // test if $char could be appended to current line
             if (mb_strwidth($line.$char, 'utf8') <= $width) {
                 $line .= $char;
@@ -1158,6 +1162,7 @@ class Application implements ResetInterface
             // if not, push current line to array and make new line
             $lines[] = str_pad($line, $width);
             $line = $char;
+            }
         }

@paxal
Copy link
Contributor Author

paxal commented Jul 26, 2019

Tested with 1000 : 1337 ms / 8 MB
Tested with 10000 : 1002 ms / 8 MB

Which value do you want me to apply for the patch ?

@nicolas-grekas
Copy link
Member

10k is good

@paxal
Copy link
Contributor Author

paxal commented Jul 26, 2019

I think we're good 👍 Thanks @nicolas-grekas

@xabbuh
Copy link
Member

xabbuh commented Jul 26, 2019

For 4.4?

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 26, 2019
@chalasr chalasr changed the base branch from master to 4.4 July 27, 2019 03:02
@chalasr
Copy link
Member

chalasr commented Jul 27, 2019

Thank you @paxal.

@chalasr chalasr merged commit 47ffbad into symfony:4.4 Jul 27, 2019
chalasr pushed a commit that referenced this pull request Jul 27, 2019
…ing long exception (paxal)

This PR was submitted for the master branch but it was merged into the 4.4 branch instead (closes #32736).

Discussion
----------

[Console] avoid using huge amount of memory when formatting long exception

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none
| License       | MIT

When formatting exceptions, `preg_split('//u')` is used to iterate over utf8 characters. When the exception is long, the amount of memory used is huge and can reach `memory_limit`.

This PR uses a `Generator` to iterate over the string instead of splitting it, thus reducing the amount of memory.

Commits
-------

47ffbad Avoid using huge amount of memory when formatting long exception
@paxal paxal deleted the console/error_formatting branch July 29, 2019 12:10
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
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.

None yet

6 participants