Container - stripComments can still use a huge amount of memory

[peteward]

Hi,

A while ago I contributed on trying to reduce memory consumption in building the container through #17377. This impacted me because I use Platform.sh with Sylius, a heavy application in a limited RAM environment.

However, despite the above PR, a growth in our Sylius application meant that we recently started swapping on container compile and could not deploy to Platform.sh because any attempt at running a console command would be killed after 5 mins of swapping.

After some investigation it is simply token_get_all here.

In our application, checking memory consumption either side of this line on PHP 7.0.3:

21:33:54 pre token_get_all  49MB 
21:33:55 post token_get_all 341MB 

That's just under 300MB to tokenize the container, I'm not sure how much it is on php5.X but it's likely to be more.

The workaround I've used for our application was to override the Kernel methods and use php_strip_whitespace instead:

    /**
     * The following 2 methods workaround the enormous memory usage of token_get_all() which is used in
     * stripComments() below.
     *
     * Instead of calling token_get_all() we use php_strip_whitespace() which is actually more aggressive stripping
     * and results in a smaller cached container.
     *
     * However this can only be run against a file (not a string).
     * So we dump the container as normal without any stripping, then afterwards strip and re-save the container
     *
     * This resulted in a decrease in total memory consumption of the process that builds the container
     * from 350mb to <50mb in PHP7, which will allow it to be built on Platform.
     *
     * PW, 03/2016
     *
     * {@inheritdoc}
     */
    public function dumpContainer(ConfigCache $cache, ContainerBuilder $container, $class, $baseClass)
    {
        parent::dumpContainer($cache, $container, $class, $baseClass);

        if (!$this->debug) {
            $cache->write(php_strip_whitespace($cache->getPath()), $container->getResources());
        }
    }

    /**
     * {@inheritdoc}
     */
    public static function stripComments($source)
    {
        return $source;
    }

The memory consumption after dumping the container now was only an extra 10MB compared to 290MB with token_get_all. The time difference was fairly negligible, slightly slower though. The resulting container was 4.8MB compared to 5.6MB.

Clearly we are a heavier use-case here, probably one of the larger containers around, but as systems and platforms grow I could see more people in our situation.

The original comment on the stripComments() method is:

     * We don't use the PHP php_strip_whitespace() function
     * as we want the content to be readable and well-formatted.

My questions are:

• Seeing as stripComments is only being used when debug is false anyway (usually in the production environment) how important is it that this file is readable? I understand there could be a use-case for trying to debug and manually change this file - maybe?
• If it's really important to keep it readable then maybe there's a more memory-efficient way of stripping while keeping readability that doesn't consume such an enormous amount of memory on large strings?
• Does stripping really matter anyway since you would hope now that everyone using Symfony is using opcache and any savings here will be irrelevant in production anyway (I think...)?

If you want to keep the current approach then I would be more than happy to contribute a Cookbook chapter about this since it took me a while to work around this.

[linaori]

Isn't this related? https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Kernel.php#L717

[peteward]

That was my initial contribution to release memory for processing after this stripComments, but it's irrelevant if token_get_all itself can eat 290mb in a single php func call and push you into swap.

[jakzal]

The only issue I see with using php_strip_whitespaces() is that it puts everything in a single line. This could potentially cause debugging problems. Any problem with the container would be logged as caused in line 1.

[peteward]

yes indeed, if you do need to debug your container in production that's the downside - and most likely the reason why it wasn't used originally.

[linaori]

Except that debugging is discouraged in production

[jakzal]

By "debugging" i meant analysing logs more than actually debugging.

[linaori]

How often would you actually need the "prod" container? I think it's neglectable, worst case scenario you can throw a prettyfier over it.

[javiereguiluz]

Another potential issue is that putting the entire container in one line could make it fail for complex containers (I guess PHP limits this length).

[linaori]

@javiereguiluz did some googling but I don't think this matters for PHP. Line endings are nothing more than a whitespace character and probably stripped away.

[peteward]

I thought the second question was more interesting - Is it even worth stripping the container if opcache is pretty standard now and Symfony 3 requires >= PHP 5.5.9 anyway?

Anyone using PHP in production nowadays without Opcache has probably got more to worry about than just a stripped container...

Based on my minimal knowledge of opcache, I would vote for just not stripping at all.

Does anyone more informed than my have any thoughts?

[jakzal]

@peteward I guess we need a performance test to see what's the impact. Since you've already got an app with a huge container, perhaps it'd be easy for you to perform a comparision with blackfire?

Btw, docblocks wouldn't be stripped unless opcache.save_comments is set to false. However, for applications using annotations it needs to be set to true.

[peteward]

OK that was the more informed point right there 👍 I was thinking comments would be stripped as standard from opcache but annotations are a good reason why not to.

Still, the containers comment blocks are all very formulaic:

    /**
     * Gets the 'X' service.
     *
     * This service is shared.
     * This method always returns the same instance of the service.
     *
     * @return \Y A \Z instance.
     */

There should be more memory efficient ways of stripping these out and leaving the rest to opcache.

I will try to do some performance tests this week.

[nicolas-grekas]

@peteward could you please check #18048

[peteward]

looks good, will give it a try later.

[linaori]

That's actually a very nice work-around, 👍 for the idea!