[DomCrawler] Fix memory leak with filter method #11221

jderusse · 2014-06-24T21:59:32Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#10879
License	MIT
Doc PR	no

Add a public method destroy on Translator to reset properties and free memory

romainneutron · 2014-06-24T22:02:32Z

src/Symfony/Component/DomCrawler/Tests/CrawlerTest.php

+            $crawler->filter('tr > td');
+        }
+
+        $after = memory_get_usage(true);


to get an accurate estimation of the memory used, you should call gc_collect_cycles before memory_get_usage

You're right, but gc_collect_cycles also solve (partialy) the circular reference memory leak. I don't use it, because I want the test prove that there is a issue when GC is not called,

jderusse · 2014-06-25T05:54:23Z

src/Symfony/Component/CssSelector/CssSelector.php

@@ -61,8 +61,10 @@ public static function toXPath($cssExpr, $prefix = 'descendant-or-self::')
            ->registerParserShortcut(new ClassParser())
            ->registerParserShortcut(new HashParser())
        ;
+        $resp = $translator->cssToXPath($cssExpr, $prefix);
+        $translator->destroy();


An other solution to solve this meomry leak, is to replace this line by a simple.

if (gc_enabled()) { gc_collect_cycles(); }

Wich solution do you think is the better ?

Using the garbage collector sounds like a good option when available.

relyinbg on the garbage collector would indeed be better (calling destroy on a translator puts it in a non-working state, so I would prefer avoiding to add it in the API)

fabpot · 2014-06-25T06:04:23Z

src/Symfony/Component/CssSelector/XPath/Translator.php

@@ -85,6 +85,21 @@ public function __construct(ParserInterface $parser = null)
    }

    /**
+     * Resets local properties to there initial values
+     */
+    public function destroy()


reset() sounds better.

Couldnt this just be a destructor?

if you mean __destructor, the answer is no, because the __destructor is not called due to circular reference.
If you suggest the name destructor, I'm not aggre, because this method does not destruct anything. @fabpot is right, reset is better name.

apfelbox · 2014-06-25T07:06:57Z

@jeremy-derusse could you ~~add~~ do some benchmarks? I can imagine that invoking the GC every time could be quite expensive.

jderusse · 2014-06-25T07:51:59Z

@apfelbox In this article http://alexatnet.com/articles/optimize-php-memory-usage-eliminate-circular-references gc_collect_cycles seems to be as expensive as ->destroy()

In our case

for ($i = 0; $i < 10000; $i++) {
    $crawler = new Crawler($html);
    $crawler->filter('tr > td');
}

with call to ->destroy() => 2.9146590232849
with call to gc_collect_cycles() => 3.0352509021759

jpauli · 2014-06-25T08:10:38Z

gc_collect_cycles() only collects circular references. So it is useless if you don't have circular references. PHP will clean variables when they become out of scope, automatically

nicolas-grekas · 2014-06-25T08:39:30Z

gc_collect_cycles() can't really be benchmarked, because it depends on the recent runtime history of the PHP engine. When collecting a simple object graph, it will be fast. But it can grow really expensive with complex structures I believe. It'd prefer letting the PHP engine do this on his own.
What we can and should do is removing circular references IMHO, nothing more.

stof · 2014-06-25T09:01:28Z

@jpauli the issue here is precisely that the object graph of the CssSelector translator is cyclic

jderusse · 2014-06-25T09:18:34Z

@jpauli here is the circular reference: https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/CssSelector/XPath/Translator.php#L79
https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/CssSelector/XPath/Extension/NodeExtension.php#L50

Thank you @nicolas-grekas, I miss this point in my benchmark. Regarding those arguments, it's not a good idea to use gc_collect_cycles, I'll just rename destroy into reset as suggested by @fabpot

stof · 2014-06-25T09:38:51Z

@jeremy-derusse exposing reset() in the translator is not a good diea either, because it puts it in a non-working state. The right fix would be to refactor the logic to eliminate the circular reference. I have an idea for this but I don't have time to implement it just now. I will probably have to wait until tomorrow evening at least

stof · 2014-06-26T23:26:38Z

see #11242 for my own attempt at fixing it, without altering the external API of the Translator and without putting it in an invalid state

…cular object graph (stof) This PR was merged into the 2.3 branch. Discussion ---------- [CssSelector] Refactored the CssSelector to remove the circular object graph | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10879, replaces #11221 | License | MIT | Doc PR | n/a This allows the translator and its extensions to be garbage collected based on the refcount rather than requiring the garbage collector run, making it much more likely to happen at the end of the ``CssSelector::toXPath`` call. Node translators now receive the Translator as second argument, instead of requiring to inject it in the extension to keep a reference to it. This way, the Translator is referenced nowhere inside it, only by the caller, and so will be destructed at the end of the usage (and extensions will then be destructed after it when not used anymore). Commits ------- 994f81f Refactored the CssSelector to remove the circular object graph

romainneutron reviewed Jun 24, 2014
View reviewed changes

jderusse changed the title ~~Fix DomCrawler memory leak with filter method~~ [DomCrawler] Fix memory leak with filter method Jun 24, 2014

jderusse mentioned this pull request Jun 25, 2014

DomCrawler memory leak with filter method #10879

Closed

jderusse reviewed Jun 25, 2014
View reviewed changes

Jérémy Derussé added 2 commits June 25, 2014 07:57

Free variables to eliminate circular reference

06e1dc3

Run GC before measures

3d73503

fabpot reviewed Jun 25, 2014
View reviewed changes

stof mentioned this pull request Jun 26, 2014

[CssSelector] Refactored the CssSelector to remove the circular object graph #11242

Merged

jderusse closed this Jun 27, 2014

apfelbox mentioned this pull request Jun 27, 2014

Symfony 2.5 leaks memory during functional tests #11236

Closed

jderusse deleted the domcrawler-memory-leak branch May 1, 2017 15:41

Uh oh!

[DomCrawler] Fix memory leak with filter method #11221

[DomCrawler] Fix memory leak with filter method #11221

Uh oh!

Conversation

jderusse commented Jun 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apfelbox commented Jun 25, 2014

Uh oh!

jderusse commented Jun 25, 2014

Uh oh!

jpauli commented Jun 25, 2014

Uh oh!

nicolas-grekas commented Jun 25, 2014

Uh oh!

stof commented Jun 25, 2014

Uh oh!

jderusse commented Jun 25, 2014

Uh oh!

stof commented Jun 25, 2014

Uh oh!

stof commented Jun 26, 2014

Uh oh!

Uh oh!