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

Dramatically reduce memory usage by using classes instead of object-like arrays #710

Merged
merged 3 commits into from
Jun 25, 2019

Conversation

maks-rafalko
Copy link
Member

All proofs are here: https://steemit.com/php/@crell/php-use-associative-arrays-basically-never

  • Objects of DTO classes takes less memory in PHP and as performant as arrays (or even better).
  • Moreover, instead of array typehints we now have a real classes or typed collections, which is a big win IMO.
  • No more passed variable by reference (&$coverage)

This PR follows #708 as @sanmai requested, I just extracted it to another PR for easier reviews.

Partially fixes issue #705


What is the memory benefit?

  1. Originally, I wasn't able to run Infection for Psalm with memory_limit=12G
  2. with Reduce memory usage for Coverage Data #708, I was able to run Infection for Psalm with memory_limit=12G
  3. With this PR, I'm able to run Infection for Psalm with memory_limit=5G

So it is 7Gb or 58% less memory usage for Psalm case.

@maks-rafalko maks-rafalko added this to the 0.14.0 milestone Jun 20, 2019
bdsl added a commit to bdsl/psalm that referenced this pull request Jun 20, 2019
bdsl added a commit to bdsl/psalm that referenced this pull request Jun 20, 2019
return $self;
}

public static function with(string $testMethod, string $testFilePath, float $time): self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what i can see this method isn't used outside of the tests. Meaning we can probably remove this, as well as the properties that it sets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

properties are used here https://github.com/infection/infection/pull/710/files#diff-d0cc0b68f2e397d7080d2f7c58834ce3R236, we can't remove them.

Test method still used in tests only though

@bdsl
Copy link
Contributor

bdsl commented Jun 20, 2019

I had a go at using this in CircleCI to test Psalm. Installed from commit ec775c9 via composer. Unfortunately I'm still getting the same sort of failures.

Strangely it took less time to fail when I increased the memory limit, but I suspect that may just be random variation:

https://circleci.com/gh/bdsl/psalm/494
https://circleci.com/gh/bdsl/psalm/495

@theofidry
Copy link
Member

I would be interesting to run a blackfire profile to see the memory difference. That said, since the difference is still easy to see even without profiling I'd say it's more out of curiosity than a necessity

@theofidry
Copy link
Member

@bdsl a good profiling could help out, but it's also very likely that the coverage data is just horribly big. We can work on making it better, but ultimately you do need more resources or alternatively leverage incremental mutations, i.e. filter out the unchanged files, instead of running Infection against the whole codebase all the time

@gmponos
Copy link
Contributor

gmponos commented Jun 22, 2019

Does this improve speed as well?

@sanmai
Copy link
Member

sanmai commented Jun 22, 2019

It may or may not, but this is not the place where Infection spend most of the time. So that the overall difference in run times excepted to negligible.

…ve DX, typehints, and the codebase in general

Everything is based on this article: https://steemit.com/php/@crell/php-use-associative-arrays-basically-never

Objects of DTO classes takes less memory in PHP and as performant as arrays.
Moreover, instead of `array` typehint we now have a real classes or typed collections
@maks-rafalko
Copy link
Member Author

@sidz done

Copy link
Member

@sidz sidz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me

@theofidry theofidry merged commit 4c68bc3 into master Jun 25, 2019
@theofidry theofidry deleted the objects-instead-of-arrays branch June 25, 2019 08:25
@theofidry
Copy link
Member

Cool one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants