Memory leak when self-referencing #3661

Closed
sannysoft opened this Issue Sep 4, 2014 · 12 comments

Comments

Projects
None yet
5 participants
@sannysoft

Found the following problems when working with Yii2 events - it stores [$this,'functionName'] in object events array to call it with call_user_func later - it leads to memory leak and termination.

Here is a test case:

<?php

class TestClass
{
    private $_events = [];

    public function __construct()
    {
        $this->_events[] = $this;
    }
}

while (1)
    $hotel = new TestClass();
@andrey-yantsen

This comment has been minimized.

Show comment
Hide comment

👍

@sannysoft

This comment has been minimized.

Show comment
Hide comment
@sannysoft

sannysoft Sep 5, 2014

Guys, can anybody please look into this? The bug is preventing us from using Yii2 (and probably many other frameworks) with hhvm.

Guys, can anybody please look into this? The bug is preventing us from using Yii2 (and probably many other frameworks) with hhvm.

@paulbiss paulbiss self-assigned this Sep 9, 2014

@paulbiss

This comment has been minimized.

Show comment
Hide comment
@paulbiss

paulbiss Sep 10, 2014

Contributor

PHP 5 has had a mechanism for collecting cyclic references since 5.3.0. We don't currently support this, any cycles are held until the end of the request.

http://php.net/manual/en/features.gc.collecting-cycles.php

Contributor

paulbiss commented Sep 10, 2014

PHP 5 has had a mechanism for collecting cyclic references since 5.3.0. We don't currently support this, any cycles are held until the end of the request.

http://php.net/manual/en/features.gc.collecting-cycles.php

@sannysoft

This comment has been minimized.

Show comment
Hide comment
@sannysoft

sannysoft Sep 10, 2014

@paulbiss Any plans to implement this?

@paulbiss Any plans to implement this?

@sannysoft

This comment has been minimized.

Show comment
Hide comment
@sannysoft

sannysoft Sep 10, 2014

Or is there a way to clean this references manually?

Or is there a way to clean this references manually?

@paulbiss

This comment has been minimized.

Show comment
Hide comment
@paulbiss

paulbiss Sep 10, 2014

Contributor

It's a feature that we used to support but was removed because no one was maintaining it. There isn't currently a lot of interest in resurrecting it. I think @ptarjan has some experience with removing cyclic reference patterns from frameworks.

Contributor

paulbiss commented Sep 10, 2014

It's a feature that we used to support but was removed because no one was maintaining it. There isn't currently a lot of interest in resurrecting it. I think @ptarjan has some experience with removing cyclic reference patterns from frameworks.

@sannysoft

This comment has been minimized.

Show comment
Hide comment
@sannysoft

sannysoft Sep 11, 2014

@ptarjan Can you help us to solve the problem in HHVM? Probably many people experience memory leaks while they don't even know about that. If anybody at least have a reference to the function in object (like [$this,'fnc']) - he will be influenced by the problem.

@ptarjan Can you help us to solve the problem in HHVM? Probably many people experience memory leaks while they don't even know about that. If anybody at least have a reference to the function in object (like [$this,'fnc']) - he will be influenced by the problem.

@augusteiner

This comment has been minimized.

Show comment
Hide comment
@augusteiner

augusteiner Sep 11, 2014

I think this is a problem most apps out there (not only PHP) have. Memory management is a boring task to do with modern languages, but it is a must. A way to less the pain is to implement some memory release methods/patterns within classes that heavily use cyclic references.

A way to do this would be using iterators/generators for loops (or doing this by hand :/).

Here is a simplistic example (tested with PHP 5.5.16):

class TestClass
{
    private $_events = [];
    public function __construct()
    {
        $this->_events[] = $this;
    }
    public function __destruct()
    {
        $this->Dispose(false);
    }
    public function Dispose($disposing = true)
    {
        // echo 'Disposing...';

        $this->_events = null;
    }
}

function Instances()
{
    while (true)
    {
        $hotel = new TestClass();

        yield $hotel;

        $hotel->Dispose();
    }
}

// ugly thing to see the line breaks in browsers
echo '<pre>';

$i = 0;

foreach (Instances() as $hotel)
{
    // don't be too much verbose
    if ($i++ % 10000 == 0)
    {
        echo (memory_get_usage(true) / 1024 / 1024) . "MB\n";
    }

    // var_dump($hotel);
    // die;
}

I think this is a problem most apps out there (not only PHP) have. Memory management is a boring task to do with modern languages, but it is a must. A way to less the pain is to implement some memory release methods/patterns within classes that heavily use cyclic references.

A way to do this would be using iterators/generators for loops (or doing this by hand :/).

Here is a simplistic example (tested with PHP 5.5.16):

class TestClass
{
    private $_events = [];
    public function __construct()
    {
        $this->_events[] = $this;
    }
    public function __destruct()
    {
        $this->Dispose(false);
    }
    public function Dispose($disposing = true)
    {
        // echo 'Disposing...';

        $this->_events = null;
    }
}

function Instances()
{
    while (true)
    {
        $hotel = new TestClass();

        yield $hotel;

        $hotel->Dispose();
    }
}

// ugly thing to see the line breaks in browsers
echo '<pre>';

$i = 0;

foreach (Instances() as $hotel)
{
    // don't be too much verbose
    if ($i++ % 10000 == 0)
    {
        echo (memory_get_usage(true) / 1024 / 1024) . "MB\n";
    }

    // var_dump($hotel);
    // die;
}
@paulbiss

This comment has been minimized.

Show comment
Hide comment
@paulbiss

paulbiss Sep 11, 2014

Contributor

@augusteiner I don't think that snippet is doing what you expect it to. You're creating new generators every time you call Instance but not actually running any of them.

Contributor

paulbiss commented Sep 11, 2014

@augusteiner I don't think that snippet is doing what you expect it to. You're creating new generators every time you call Instance but not actually running any of them.

@augusteiner

This comment has been minimized.

Show comment
Hide comment
@augusteiner

augusteiner Sep 12, 2014

Ok, not tested the code, sorry, my bad :p (snipet updated)

The point is that some would mind this would get done automatically (without the Dispose method), but that is not the way the php.net implementation behaves (because of the cyclic reference) and the generator thing is about not calling Dispose method everywhere. But, of course, in some scenarios this cannot be accomplished and the Disposal method have to be called "explicitly".

This approach dispenses ugly gc_* functions which i consider as a bad practice, and as far as i remember some are not implemented in hhvm.

I think these type of concerns about memory management should get stronger, mainly in PHP development (having in mind this odd PHP behavior about cyclics).

Ok, not tested the code, sorry, my bad :p (snipet updated)

The point is that some would mind this would get done automatically (without the Dispose method), but that is not the way the php.net implementation behaves (because of the cyclic reference) and the generator thing is about not calling Dispose method everywhere. But, of course, in some scenarios this cannot be accomplished and the Disposal method have to be called "explicitly".

This approach dispenses ugly gc_* functions which i consider as a bad practice, and as far as i remember some are not implemented in hhvm.

I think these type of concerns about memory management should get stronger, mainly in PHP development (having in mind this odd PHP behavior about cyclics).

@paulbiss paulbiss added the memory leak label Dec 8, 2014

@paulbiss paulbiss assigned edwinsmith and unassigned paulbiss Dec 8, 2014

@paulbiss

This comment has been minimized.

Show comment
Hide comment
@paulbiss

paulbiss Jul 27, 2016

Contributor

We now have support for enabling the GC

Contributor

paulbiss commented Jul 27, 2016

We now have support for enabling the GC

@paulbiss paulbiss closed this Jul 27, 2016

@edwinsmith

This comment has been minimized.

Show comment
Hide comment
@edwinsmith

edwinsmith Aug 1, 2016

Contributor

-vEval.EnableGC=1 on the commandline enables automatic backup mark/sweep collection, as well as enabling gc_collect_cycles() to actually run an on-demand collection. Unlike PHP7, HHVM's implementation of gc_collect_cycles() does not count the number of cycles.

Contributor

edwinsmith commented Aug 1, 2016

-vEval.EnableGC=1 on the commandline enables automatic backup mark/sweep collection, as well as enabling gc_collect_cycles() to actually run an on-demand collection. Unlike PHP7, HHVM's implementation of gc_collect_cycles() does not count the number of cycles.

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