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

form: CachingFactoryDecorator consumes unbounded amounts of memory #29259

Open
pschultz opened this Issue Nov 19, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@pschultz

pschultz commented Nov 19, 2018

Symfony version(s) affected: >= 2.7

Description
CachingFactoryDecorator has no bounds on its caches, leading to excessive memory usage.

One of our console commands builds several thousand form instances to generate a sitemap (the query strings are inferred from the form values).

Because the CachingFactoryDecorator holds on to every choice list ever built, the command's memory usage easily exceeds 500 MB. The problem goes away by changing the form.choice_list_factory from form.choice_list_factory.cached (the hard coded default) to form.choice_list_factory.default:

services:
    form.choice_list_factory: '@form.choice_list_factory.default'

How to reproduce
Run the following console command and watch the memory usage increase forever:

<?php

use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Helper\ProgressBar;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
use Symfony\Component\Form\Extension\Core\Type\FormType;

class EatMemoryCommand extends Command
{
    protected function configure()
    {
        $this
            ->setName('eat-memory')
            ->setDescription('Consumes all available memory by repeatedly building forms')
        ;
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $formFactory = $this->getApplication()->getKernel()->getContainer()->get('form.factory');

        $progress = new ProgressBar($output);
        $progress->setFormat('%current% / memory usage: %memory% ');
        $progress->setRedrawFrequency(1e3);

        for (;;) {
            $builder = $formFactory->createBuilder(FormType::class, [], []);
            $builder->add('foo', ChoiceType::class, [
                'choices' => [ 'bar' => 'baz', ],
            ]);
            $builder->getForm();

            $progress->advance();
        }
    }
}

Possible Solution
CachingFactoryDecorator should limit the lengths of its lists and views properties.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Nov 27, 2018

@pschultz any suggestions how to deal with the limits in CachingFactoryDecorator?

@pschultz

This comment has been minimized.

pschultz commented Nov 28, 2018

LRU is as good as any strategy, I suppose. In createListFromLoader, for instance:

$hash = self::generateHash(array($loader, $value), 'fromLoader');

if (isset($this->lists[$hash])) {
    $list = $this->lists[$hash];
} else {
    $list = $this->decoratedFactory->createListFromLoader($loader, $value);
}

// move $hash to end of list for LRU semantics
unset($this->lists[$hash]);
$this->lists[$hash] = $list;

// remove least recently used lists if necessary
while (count($this->lists) > 1000) {
    array_shift($this->lists);
}
@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Nov 28, 2018

👍 1000 seems arbitrary.. but we can inject it.

@stof

This comment has been minimized.

Member

stof commented Nov 28, 2018

your case of eat-memory looks weird to me. As choices are the same, the CachingFactoryDecorator should have only 1 list in its cache here.

I suspect that your memory leak is actually related to the profiler instrumentation of forms, which retains data about all created forms.
Enabling the profiler instrumentation is known to cause memory leaks in long running processes, because this instrumentation is precisely meant to keep data in memory to be able to store the profile.

@stof

This comment has been minimized.

Member

stof commented Nov 28, 2018

and disabling instrumentation automatically for the CLI would be harder, as the cached container is shared between all SAPI (otherwise, warming up the cache would be a pain, as you would have to warm it up using PHP-FPM to get the right cache). So it would require to make the instrumentation itself becoming a no-op at runtime instead. And that would break support for php-pm and similar projects, which run the webserver using a long-running CLI script (for which the profiler instrumentation memory leak is solved by resetting stateful services between requests, as each request being handled is not long running).

@pschultz

This comment has been minimized.

pschultz commented Nov 28, 2018

@ro0NL, yes, 1000 was totally arbitrary. I don't have an intuition for an appropriate default.

@stof, yes, the memory usage in the reproducer above is caused by the profiler. My bad. Our real command runs with --env=prod --no-debug (i.e. without profiler) and creates many (thousands) different choice lists.

This would be closer to the real code:

$builder->add('foo', ChoiceType::class, [
    'choices' => [ 'bar' => uniqid() ],
]);
@stof

This comment has been minimized.

Member

stof commented Nov 28, 2018

hmm, actually, my comment about the resetting for phppm is wrong. CachingFactoryDecorator does not implement the ResetInterface currently. It should

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