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

Add a basic test for pantsd memory leaks. #7488

Merged
merged 2 commits into from Apr 4, 2019

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

commented Apr 3, 2019

Problem

See #7439.

Solution

Use psutil to get the current memory usage of the daemon under test. Even pre-#7390, memory usage already seems to grow roughly linearly by about 3MB per run for this particular command.

Fixes #7439.

@stuhood stuhood requested review from illicitonion, ity and blorente Apr 3, 2019

@blorente
Copy link
Contributor

left a comment

I think I'd rather have a test where we check the increase is below a threshold run after run (as opposed to only at the end), but I fear that it might lead to flakiness.

Thanks!

number_of_runs = 10
max_memory_increase_fraction = 0.4
with self.pantsd_successful_run_context() as (pantsd_run, checker, workdir, config):
cmd = ['list', 'testprojects::']

This comment has been minimized.

Copy link
@blorente

blorente Apr 3, 2019

Contributor

I'm not sure, but it may happen that some target in testprojects does not parse (this is why, for instance, we ignore .*/testprojects/.* in the contrib section of build-support/bin/ci.sh). Maybe this would better be examples?

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 3, 2019

Author Member

Everything in the repo is parseable, although it is not all buildable.

)
)

increase_fraction = (final_memory_usage / initial_memory_usage) - 1.0

This comment has been minimized.

Copy link
@blorente

blorente Apr 3, 2019

Contributor

Is this a place where integer division can mean we los precision?

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 3, 2019

Author Member

Hah. Apparently this changed between python 2 and 3, and I was thinking of the 3 behaviour.

Python 2.7.10 (default, Feb 22 2019, 21:17:52)
..
>>> 3/4
0
Python 3.7.0 (default, Jul 23 2018, 20:22:55)
..
>>> 3/4
0.75

Will fix.

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 3, 2019

Author Member

Oh, nevermind. In this file:

from __future__ import absolute_import, division, print_function, unicode_literals

Which explains why I was able to see this test fail before I made it pass.

Show resolved Hide resolved tests/python/pants_test/pantsd/test_pantsd_integration.py Outdated
@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

40% is a pretty large increase, and suggests we're leaking 3-4% per run, when ideally we'd expect the number to be 0. Is that what we're currently observing (and we'd like to not regress beyond that), or is there a rationale for the limit?

@ity
Copy link
Contributor

left a comment

thanks for adding the test!

def test_pantsd_memory_usage(self):
"""Validates that after N runs, memory usage has increased by no more than X percent."""
number_of_runs = 10
max_memory_increase_fraction = 0.4

This comment has been minimized.

Copy link
@ity

ity Apr 3, 2019

Contributor

does this fail when < 0.4?

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 3, 2019

Author Member

Yea.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

40% is a pretty large increase, and suggests we're leaking 3-4% per run, when ideally we'd expect the number to be 0. Is that what we're currently observing (and we'd like to not regress beyond that), or is there a rationale for the limit?

This is what we are currently observing, unfortunately. As we enable pantsd without forking, we can begin to chase these down, but I expect the largest contributors to change when we stop forking, and so that would likely be the best time to investigate... possibly via a TODO that bumps this limit and begins chasing leaks in a followup.

I have done no investigation at all into what is being leaked here.

@illicitonion
Copy link
Contributor

left a comment

Looks good :) Thanks!

@ity

ity approved these changes Apr 4, 2019

@stuhood stuhood merged commit 00fd117 into pantsbuild:master Apr 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/pantsd-memory-usage branch Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.