Cleanup unused cffi handles to free memory #4135

Merged
merged 16 commits into from Dec 21, 2016

Conversation

Projects
None yet
4 participants
@stuhood
Member

stuhood commented Dec 10, 2016

Problem

Previously, the set of CFFI handles in the ExternContext would grow unboundedly as they were created, amounting to all Values that were used (and which later became unused) being leaked.

Solution

To allow CFFI handles to be collected, Clone and Copy were removed from the rust Value type, which put restrictions on who could create/copy Values. Drop was then implemented for Value such that a Value going out of scope caused its internal handle value to be enqueued to later have its underlying handle dropped on the python side (potentially allowing it to be garbage collected).

It's unavoidable that multiple CFFI handles will exist per underlying object, because we don't want to hash all Values to dedupe them (we only hash objects that we are forced to due to lifting to a Key). Because of this, doing reference counting on the rust side (although easy enough) would have been confusing due to both reference counting and multiple copies existing.

Result

Memory previously leaked in ExternContext._handles should be periodically freed instead.

(TODO: benchmarks to follow)

@stuhood stuhood requested review from baroquebobcat, jsirois and JieGhost Dec 10, 2016

@kwlzn

kwlzn approved these changes Dec 19, 2016

lgtm!

src/rust/engine/src/nodes.rs
@@ -212,17 +220,17 @@ impl Select {
variant_value: Option<&str>
) -> Option<Value> {
// Check whether the subject is-a instance of the product.
- if let Some(&candidate) = self.select_literal_single(context, candidate, variant_value) {
- return Some(candidate)
+ if let Some(&ref candidate) = self.select_literal_single(context, candidate, variant_value) {

This comment has been minimized.

@JieGhost

JieGhost Dec 20, 2016

Contributor

Is it just same as "...Some(candidate)..."?
My understanding is select_literal_single returns Option<&Value>, thus Some(X), X is already &Value type.
Some(&X) makes X be Value type, Some(&ref X) makes X &Value type again.

@JieGhost

JieGhost Dec 20, 2016

Contributor

Is it just same as "...Some(candidate)..."?
My understanding is select_literal_single returns Option<&Value>, thus Some(X), X is already &Value type.
Some(&X) makes X be Value type, Some(&ref X) makes X &Value type again.

This comment has been minimized.

@stuhood

stuhood Dec 20, 2016

Member

This is called "destructuring", also known as pattern matching. I'm also destructuring the value out of the Some. In the case of destructuring, you can think of & as having the opposite effect: I'm destructuring the & off of the value... ie, dereferencing it. So in both of the cases here, the destructuring is about unwrapping something: the Some, and the reference.

@stuhood

stuhood Dec 20, 2016

Member

This is called "destructuring", also known as pattern matching. I'm also destructuring the value out of the Some. In the case of destructuring, you can think of & as having the opposite effect: I'm destructuring the & off of the value... ie, dereferencing it. So in both of the cases here, the destructuring is about unwrapping something: the Some, and the reference.

This comment has been minimized.

@JieGhost

JieGhost Dec 20, 2016

Contributor

I understand it is destructing and "&" means dereferencing here. I am just wondering whether
"Some(&ref candidate)" and "Some(candidate)" are equivalent here.

@JieGhost

JieGhost Dec 20, 2016

Contributor

I understand it is destructing and "&" means dereferencing here. I am just wondering whether
"Some(&ref candidate)" and "Some(candidate)" are equivalent here.

This comment has been minimized.

@stuhood

stuhood Dec 20, 2016

Member

Ahh, I see what you mean here. You're totally right. Dropped the &ref. It's interesting that the compiler doesn't warn that that is a noop.

@stuhood

stuhood Dec 20, 2016

Member

Ahh, I see what you mean here. You're totally right. Dropped the &ref. It's interesting that the compiler doesn't warn that that is a noop.

@JieGhost

This comment has been minimized.

Show comment
Hide comment
@JieGhost

JieGhost Dec 20, 2016

Contributor

I have a general question.

Value is always created on Python side and then got passed to Rust code as return value of extern functions, right? So now since you removed Copy trait from Value struct, does that mean now when you assign (bind) the return value to a variable, the ownership is transferred as the consequence of the assignment? And previously, the struct was just copied field by field?

(assume above is correct) And with ownership of the object moved to rust code, the object will be cleaned (which means drop is also called) once the object is out of scope, even if there is still handle on the Python side?

Contributor

JieGhost commented Dec 20, 2016

I have a general question.

Value is always created on Python side and then got passed to Rust code as return value of extern functions, right? So now since you removed Copy trait from Value struct, does that mean now when you assign (bind) the return value to a variable, the ownership is transferred as the consequence of the assignment? And previously, the struct was just copied field by field?

(assume above is correct) And with ownership of the object moved to rust code, the object will be cleaned (which means drop is also called) once the object is out of scope, even if there is still handle on the Python side?

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Dec 20, 2016

Member

So now since you removed Copy trait from Value struct, does that mean now when you assign (bind) the return value to a variable, the ownership is transferred as the consequence of the assignment? And previously, the struct was just copied field by field?

@JieGhost: No: rust has move semantics by default, so the vast majority of the time, the value (and its ownership) were moved from one owner to another. Copy is used implicitly in positions where the borrow checker determines that two paths will end up using the same struct. A lot of the changes (like the one you pointed out above) were positions where we were implicitly (maybe even unnecessarily) copying this data. But I think it was still fine to have marked Value Copy, because Value is only ~16 bytes.

EDIT: I think you were right, in that variable references are always copies in rust... the Copy trait just indicates that even after something has been copied, the previous value is still available (which satisfies the borrow checker): http://stackoverflow.com/a/24253573

Member

stuhood commented Dec 20, 2016

So now since you removed Copy trait from Value struct, does that mean now when you assign (bind) the return value to a variable, the ownership is transferred as the consequence of the assignment? And previously, the struct was just copied field by field?

@JieGhost: No: rust has move semantics by default, so the vast majority of the time, the value (and its ownership) were moved from one owner to another. Copy is used implicitly in positions where the borrow checker determines that two paths will end up using the same struct. A lot of the changes (like the one you pointed out above) were positions where we were implicitly (maybe even unnecessarily) copying this data. But I think it was still fine to have marked Value Copy, because Value is only ~16 bytes.

EDIT: I think you were right, in that variable references are always copies in rust... the Copy trait just indicates that even after something has been copied, the previous value is still available (which satisfies the borrow checker): http://stackoverflow.com/a/24253573

@baroquebobcat

lgtm too!

@stuhood stuhood merged commit adf085a into pantsbuild:master Dec 21, 2016

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/cleanup-cffi-handles branch Dec 21, 2016

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Cleanup unused cffi handles to free memory (#4135)
### Problem

Previously, the set of CFFI handles in the `ExternContext` would grow unboundedly as they were created, amounting to all `Values` that were used (and which later became unused) being leaked.

### Solution

To allow CFFI handles to be collected, `Clone` and `Copy` were removed from the rust `Value` type, which put restrictions on who could create/copy Values. `Drop` was then implemented for `Value` such that a Value going out of scope caused its internal handle value to be enqueued to later have its underlying handle dropped on the python side (potentially allowing it to be garbage collected).

It's unavoidable that multiple CFFI handles will exist per underlying object, because we don't want to hash all Values to dedupe them (we only hash objects that we are forced to due to lifting to a Key). Because of this, doing reference counting on the rust side (although easy enough) would have been confusing due to both reference counting and multiple copies existing.

### Result

Memory previously leaked in `ExternContext._handles` should be periodically freed instead. Unfortunately, the additional overhead of tracking usage causes a slight (5-10%) slowdown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment