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

[BUG]: Phalcon\Support classes leak memory #16571

Closed
maxgalbu opened this issue Apr 23, 2024 · 6 comments
Closed

[BUG]: Phalcon\Support classes leak memory #16571

maxgalbu opened this issue Apr 23, 2024 · 6 comments
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@maxgalbu
Copy link
Contributor

Describe the bug
Instantiating and invoking a Phalcon/Support class make memory grow too much.
I'm using it in a forever-running CLI script.

To Reproduce

Script to reproduce the behavior:

<?php
use Phalcon\Support\HelperFactory;

echo "beginning: ".memory_get_peak_usage() / 1024 / 1024;
echo "\n";

foreach (range(1,100000) as $num) {
    $helper = new HelperFactory();
    $helper->camelize("transaction_id");
}

echo "end: ".memory_get_peak_usage() / 1024 / 1024;
echo "\n";

Output:

beginning: 0.40937042236328
end: 42.013931274414

Expected behavior
Memory should not increase so much. Same output using ucwords():

beginning: 0.40864562988281
end: 4.3784713745117

Details

  • Phalcon version: 5.6.2
  • PHP Version: 8.1.27
  • Operating System: Debian
  • Installation type: installing via package manager
  • Zephir version (if any):
  • Server: N/A (cli script)
@niden
Copy link
Sponsor Member

niden commented Apr 29, 2024

That should be normal to be honest.

The helper factory is just that, a factory that creates additional classes. When you instantiate the factory, you have one object. Once you call say camelize on it, it will create the Camelize object transparently (and the memory will increase by doing that) and then use it to produce the result. Any subsequent calls to camelize will not increase the memory since it is already instantiated.

@niden
Copy link
Sponsor Member

niden commented Apr 29, 2024

Actually I take that back. That is what should be happening but it is not. The mapper is not reusing the object.

I will sort that out shortly

@niden niden self-assigned this Apr 29, 2024
@niden niden added status: medium Medium 5.0 The issues we want to solve in the 5.0 release and removed status: unverified Unverified labels Apr 29, 2024
@niden
Copy link
Sponsor Member

niden commented Apr 29, 2024

I run the same test with some variations.

In the test you have, you are creating the helper factory and then calling camelize which in turn will create a new Camelize object. This is in the loop so you are pretty much testing what is the memory consumption of two objects being created in a loop, again and again, so this does not represent a memory leak. The better tests are as follows (to see if there is a memory leak or not)

  • Create the factory outside the loop, run the loop with the camelize in it, and see if the memory keeps increasing
  • Create the factory and call camelize outside the loop, then call camelize in the loop.

After the change I made (PR will be coming shortly) I got these results

Your test as is:

beginning: 0.41312408447266
end: 35.912994384766

Create factory outside the loop

beginning: 0.41301727294922
end: 35.903160095215

Create the factory outside the loop and call camelize, then have camelize in the loop

beginning: 0.41147613525391
end: 35.903717041016

@niden
Copy link
Sponsor Member

niden commented Apr 29, 2024

#16574

Resolved

@niden niden closed this as completed Apr 29, 2024
@maxgalbu
Copy link
Contributor Author

In the test you have, you are creating the helper factory and then calling camelize which in turn will create a new Camelize object. This is in the loop so you are pretty much testing what is the memory consumption of two objects being created in a loop, again and again, so this does not represent a memory leak.

I'm creating the helper factory but I'm also storing every instance on the same variable $helper. Shouldn't this call the destructor on the old Factory instance every time I instantiate another Factory? Why is the old instance kept in memory?

With the following userland code mimicking HelperFactory from phalcon, the memory increases linearly just like using ucwords:

<?php

echo "beginning: ".memory_get_peak_usage() / 1024 / 1024;
echo "\n";

class Camelize
{
    public function __invoke(
        string $text,
        string $delimiters = null,
        bool $lowerFirst = false
    ): string {
        if (!$delimiters) {
            $delimiters = "\n\t";
        }
        return str_replace(str_split($delimiters), "", ucwords($text, $delimiters));
    }
}

class HelperFactory
{
    private array $services = [];

    public function __call(string $name, array $arguments)
    {
        $helper = $this->newInstance($name);
        return call_user_func_array([$helper, "__invoke"], $arguments);
    }

    public function newInstance(string $name)
    {
        if (!isset($this->services[$name])) {
            $instanceName = $this->getService($name);
            $this->services[$name] = new $instanceName;
        }

        return $this->services[$name];
    }

    protected function getService($name)
    {
        return [
            "camelize"      => "Camelize",
        ][$name];
    }
}

foreach (range(1,100000) as $num) {
    $helper = new HelperFactory();
    $helper->camelize("transaction_id");
}

echo "end: ".memory_get_peak_usage() / 1024 / 1024;
echo "\n";

Output:

beginning: 0.41677856445312
end: 4.3862762451172

After the change I made (PR will be coming shortly) I got these results

Looking at these results, there seems to be no improvements at all? At least with Create factory outside the loop there should be some improvements..

@maxgalbu
Copy link
Contributor Author

maxgalbu commented May 6, 2024

@niden did you look at my comment? maybe you just mis-copied results after your fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Status: Implemented
Development

No branches or pull requests

2 participants