Fixing forced OpCache invalidation on every template include, which is resulting in fast raising wasted OpCache memory #1007

[stephanlueckl]

Fixes: https://github.com/smarty-php/smarty/issues/1007

\Smarty\Template\Compiled::loadCompiledTemplate invalidates the cache entry of a compiled template on every call (or include) as it is forced to do so. This results in fast growing wasted memory and after reaching the tipping point in a total cleanup and refresh of the opcache.

I do not think, that is necessary to force it, seeing this change as hotfix.

Nice article about wasted memory of the opcache and it's refreshment mechanism: https://www.codana.be/en/insights/php-opcache-realpath-cache-and-preloading

[wisskid]

I really wonder why this line was in here in the first place. I guess it was already present in Smarty 4, which makes me wonder if this actually fixes #1007. If so, this must mean there some unintended change in the flow between v4 and v5.

[stephanlueckl]

@wisskid You are right, this fixes only the symptom of raising wasted memory. The Smarty5 method \Smarty\Template\Compiled::loadCompiledTemplate is the same like in Smarty4

The root cause is in the different call mechanism between Smarty 4 and Smarty 5

Smarty 4 loadCompiledTemplate is only called when it was new compiled

public function process(Smarty_Internal_Template $_smarty_tpl)
    {
        $source = &$_smarty_tpl->source;
        $smarty = &$_smarty_tpl->smarty;
        if ($source->handler->recompiled) {
            $source->handler->process($_smarty_tpl);
        } elseif (!$source->handler->uncompiled) {
            if (!$this->exists || $smarty->force_compile
                || ($_smarty_tpl->compile_check && $source->getTimeStamp() > $this->getTimeStamp())
            ) {
                $this->compileTemplateSource($_smarty_tpl);
                $compileCheck = $_smarty_tpl->compile_check;
                $_smarty_tpl->compile_check = Smarty::COMPILECHECK_OFF;
                $this->loadCompiledTemplate($_smarty_tpl);
                $_smarty_tpl->compile_check = $compileCheck;
            } else {
// Loading an already compiled file (standard case)
                $_smarty_tpl->mustCompile = true;
                @include $this->filepath;
// after the include statement mustcompile is false again 
                if ($_smarty_tpl->mustCompile) {
                    $this->compileTemplateSource($_smarty_tpl);
                    $compileCheck = $_smarty_tpl->compile_check;
                    $_smarty_tpl->compile_check = Smarty::COMPILECHECK_OFF;
                    $this->loadCompiledTemplate($_smarty_tpl);
                    $_smarty_tpl->compile_check = $compileCheck;
                }
            }
            $_smarty_tpl->_subTemplateRegister();
            $this->processed = true;
        }
    }

Smarty 5 loadCompiledTemplate is always used,

private function compileAndLoad(Template $_smarty_tpl) 
    {
        if ($_smarty_tpl->getSource()->handler->recompiled) {
            $this->recompile($_smarty_tpl);
            return;
        }

        if ($this->exists && !$_smarty_tpl->getSmarty()->force_compile
            && !($_smarty_tpl->compile_check && $_smarty_tpl->getSource()->getTimeStamp() > $this->getTimeStamp())
        ) {
// Loading an already compiled file (standard case)     
            $this->loadCompiledTemplate($_smarty_tpl);
        }

        if (!$this->isValid) {
            $this->compileAndWrite($_smarty_tpl);
            $this->loadCompiledTemplate($_smarty_tpl);
        }

        $this->processed = true;
    }

Thinking of invalidation is only necessary, when the file is changed in the process. I would suggest an optional invalidation param to the loadCompiledTemplate() method. Change will soon follow in the pull request.

As of the forced invalidation, i still think this is not necessary - if the file hasn't change, why force to reload it?

[stephanlueckl]

Fix is working for this issue, Fast growing of the wasted memory of OPCache stopped.

Update on this server went online at 16:05. No more OPCache restarts, CPU spikes stopped.

[wisskid]

Nice work @stephanlueckl !

[wisskid]

As of the forced invalidation, i still think this is not necessary - if the file hasn't change, why force to reload it?

I honestly don't have a clue, but there may be some reason both of us don't understand yet, so I'd be more comfortable to leave it in. Your PR looks great, I'm running the tests now.

[stephanlueckl]

I honestly don't have a clue, but there may be some reason both of us don't understand yet, so I'd be more comfortable to leave it in. Your PR looks great, I'm r

Okay, i've changed this line back in the last commit

[tungps250991]

@stephanlueckl Thank you for the work, it really saves us!

@wisskid I tried applying this solution & the performance issue here is gone. Could you please merge this to new v.5 release soon? It would be very helpful since we're going to release the product with Smarty upgrade by the end of this week :) Thank you!

[wisskid]

Expect a new release around Aug 15th.