Skip to content

Memory issue in v0.13 #721

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

Open
Kocal opened this issue Aug 7, 2020 · 14 comments
Open

Memory issue in v0.13 #721

Kocal opened this issue Aug 7, 2020 · 14 comments

Comments

@Kocal
Copy link
Contributor

Kocal commented Aug 7, 2020

Q A
Bug report? yes
Feature request? no
BC Break report? yes/no
RFC? yes/no
Version/Branch 0.13

Hi,

I'm still working on GraphQLBundle update from 0.11 to 0.13 on our big app at work, but I detected a memory issue while running our Behat suite tests.

It does not affect 0.11 and 0.12.

I've recorded htop output while running behat:

Do you have an idea of what can happens?
Thanks!

@mcg-web
Copy link
Member

mcg-web commented Aug 9, 2020

Hi,

InputValidator is the only big change between 0.12 and 0.13 but if you only updating you are not concern by this. Have you updated others dependencies (Symfony, Webonyx, ...) ?

@Kocal
Copy link
Contributor Author

Kocal commented Aug 9, 2020

While updating from 0.11.11 to 0.13.3, the following dependencies has been updated:

  • symfony/config, symfony/dependency-injection, symfony/event-dispatcher, symfony/filesystem, symfony/inflector, symfony/options-resolver, and symfony/property-access from 4.4.10 to 4.4.11
  • symfony/event-dispatcher-contracts from 1.1.7 to 1.1.9
  • symfony/polyfill-ctype from 1.17.1 to 1.18.1
  • symfony/service-contracts from 2.1.2 to 2.1.3
  • webonyx/graphql-php from 0.12.6 to 0.13.9

Then I downgraded to overblog/graphql-bundle from 0.13.3 to 0.12.8 but no other dependencies has been downgraded...

So it means that the issue comes from the bundle 😕

@Kocal
Copy link
Contributor Author

Kocal commented Aug 10, 2020

I've done some tests today, and this is what I found.

The memory leak also happens when running Behat tests not on the GraphQL API. For example some classical tests on our controllers (ArticleController):

  • 0.12.8:
    • features/api: 1.10GB max
    • features/articles: 1GB max
  • 0.13.3:
    • features/api: increase to 2GB, then I stop the tests
    • features/articles: increase to 1.48 GB, then tests stop themselves (all successful)

I can confirm the issue is also present in 0.13.0, so something wrong should have happens in v0.12.8...v0.13.0

@akomm
Copy link
Contributor

akomm commented Aug 11, 2020

Hi @Kocal

Did the php version remain the same?

@Kocal
Copy link
Contributor Author

Kocal commented Aug 11, 2020

Hi @akomm,

Oh my bad, I didn't say what version of PHP I'm using.
I'm using PHP 7.3.20 and it was the same version during my tests.

@akomm
Copy link
Contributor

akomm commented Aug 11, 2020

If neither your tests change, nor behat+its deps, the best bet would be to make a profile with xdebug.

A small chance also that some memory leak bug in the php version that was not triggered before is triggered after the changes, so if you swap the php version you might quickly exclude this case. Provided your dependencies allow for an increase to let's say 7.4 ...

@Kocal
Copy link
Contributor Author

Kocal commented Aug 11, 2020

I did some blackfire traces:

I really don't know what can happens, but I guess we will have to update PHP from 7.3 to 7.4... 😬 ping @tristanbes / @RomulusED69

@murtukov
Copy link
Contributor

@Kocal there are some reports in internet about memory leak in PHP 7.3

@akomm
Copy link
Contributor

akomm commented Aug 13, 2020

@murtukov yes, I have checked for this, but found only some in 7.3.2m which supposedly were fixed. However, I agree, as I also said myself, there might still be some I/you missed or some, which nobody found yet ;)

@akomm
Copy link
Contributor

akomm commented Aug 13, 2020

Actually, when I check the logs, there are dozens of memory leaks closed after 7.3.2: https://www.php.net/ChangeLog-7.php , including 7.4.0+

@akomm
Copy link
Contributor

akomm commented Aug 13, 2020

The diff php 7.3 is rather interesting. If you analyze / sort by memory usage, you find the main troublemaker is PhpFilesAdapter (symfony). It gives +211 MB by itself. Its callers down the stack each only give much smaller increase. But together they still sum up to the other +310 MB, which is significant.

This increase everywhere, rather than at the top of the (filtered) stack is a sign of there being a memory leak that affects everything. But PhpFilesAdapter is hit by it most.

A last note on this: leak for me is not only bug, but might also be accidentally caused by code or having some profiling enabled (symfony, debugger, etc.). PHP7.4 might just circumvent it with the new memory optimizations they have added. I also recall there was a feature adding some profiling for graphql in recent updates, just to name an example. Not sure though which version it was.

@theofidry
Copy link
Contributor

Little update: we're migrating a project step by step and we hit a wall with 0.13. The test suite no longer runs due to huge memory leaks.

A standard test can see deviations as high as +280% of memory usages. It's a bit late so I'm gonna stop here but from what I could collect, the issue is not so much of a higher memory consumption, but rather a leakage, and I can trace source of the problem here.

If I replace the content with:

public function __construct($name, callable $compiler, ?callable $evaluator = null)
    {
        if (null === $evaluator) {
            $evaluator = static function () {};
        }

        parent::__construct($name, $compiler, $evaluator);
    }

Then the memory leak issue is gone.

My suspicion is that you're allocating an exception which can never be garbage collected resulting in the leak.

@mcg-web
Copy link
Member

mcg-web commented Feb 3, 2023

@theofidry is this the file you are talking about ? I think we can replace this exception by a warning. Maybe you have a better solution ?

@theofidry
Copy link
Contributor

@mcg-web indeed; I just vendor hotfixed it to the old code (a static function throwing an exception) and it worked just fine. There is still a significant memory increase but not as much leak (which was a problem since quickly resulting in a bailout when running the test suite).

I am not exactly sure why, I assume because you made it an actual exception it probably captures the stack trace & everything. So maybe it's not much that the evaluator is not release (it's probably not release in any case), but more that the memory footprint is much bigger.

theofidry added a commit to theofidry/GraphQLBundle that referenced this issue May 2, 2023
Closes overblog#721.

Follow up of [my
comment](overblog#721 (comment)).

I cannot investigate further, but my guess is that exceptions are not
cheap objects. When creating an exception you also capture the context
and stack trace which means that even if the extension is unsued:

- it slows down the program because instantiating it is not cheap
- may prevent some objects to be garbage collected because referenced in
  the exception
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants