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.

Sign up for GitHub

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

Jump to bottom

dev/core#2073 Memory drain #18632

Merged
merged 1 commit into from
Sep 30, 2020
Merged

dev/core#2073 Memory drain #18632

merged 1 commit into from
Sep 30, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 29, 2020 •
edited
Loading

Overview

Fixes a memory leak within 4 unit test classes

Update -in fact this is not a true memory leak - just a case of memory hogging. It turns out phpunit keeps all classes until the run is finished - so if we have a LocationType DAO as a property of the class it is retained for the entire phpunit run

Before

api_v3_AddressTest leaks 2.9211MB of memory (UPDATE - I realise that is only the leaks on strings - not the total leak)

Screen Shot 2020-09-29 at 3 56 50 PM

After

api_v3_AddressTest 'only' leaks 2.715756 MB of memory (UPDATE - I realise that is only the leaks on strings - not the total leak)

Screen Shot 2020-09-29 at 4 31 15 PM

Technical Details

See https://lab.civicrm.org/dev/core/-/issues/2073 for a lot of detail on the methodology

Comments

@civibot
Copy link

civibot bot commented Sep 29, 2020

(Standard links)

@civibot civibot bot added the master label Sep 29, 2020
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Sep 29, 2020
@eileenmcnaughton
dev/core#2073 just tweak a memory leak
708fb7a 
This one is like civicrm#18632 but only
seems to have switched us from an xml leakage to a string leakage - it still
seems slightly better from a code quality point of view
@eileenmcnaughton eileenmcnaughton mentioned this pull request Sep 29, 2020
Merged
@totten totten added the merge ready PR will be merged after a few days if there are no objections label Sep 29, 2020
@totten
Copy link
Member

totten commented Sep 29, 2020

Wow, that's a nice job.

The patch looks like it only affects the test suite, and the tests pass. ✅

I grepped a version of universe, and locationTypeCreate() only appears to be used in civicrm-core (which makes sense - it's part of CiviUnitTestCase). ✅

Looks 'merge ready' to me. Since the discussion started with @seamuslee001 @demeritcowboy, and I'll leave it open for the moment in case they have feedback.

@eileenmcnaughton eileenmcnaughton changed the title dev/core#2073 Memory leak fix dev/core#2073 Memory leak fix (well not actually now I think about it but still an improvement) Sep 29, 2020
@eileenmcnaughton
Copy link
Contributor Author

I've realised that this a case of unnecessary memory use - not a leak probably. If I can capture memory in use after the whole class is deconstructed I can be sure that is a leak (& I'm pretty sure there IS a leak)

Things like this would still hammer memory usage with dataproviders in play

@eileenmcnaughton eileenmcnaughton changed the title dev/core#2073 Memory leak fix (well not actually now I think about it but still an improvement) dev/core#2073 Memory drain Sep 30, 2020
@seamuslee001
Copy link
Contributor

This only affects tests looks fine merging

@seamuslee001 seamuslee001 merged commit a8a6881 into civicrm:master Sep 30, 2020
@seamuslee001 seamuslee001 deleted the leak branch September 30, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @totten @seamuslee001