[MRG] Avoid reference cycles in Tree #2790
Conversation
|
On my box, using @ogrisel script, I cannot reproduce the leak. |
|
Regarding So overall, I am +1 for this. Any second opinion? Thanks for the patch! |
|
1000 trees fit on 50 features: Predicting 1e5 samples, best of 3 Similar 2-3% time increase for smaller samples. I assume this is not big, but not insignificant..? We can in any case accept this fix and speed up |
Whoops :)
+1 for that. |
|
Weird, I still get the leak when I run the script of #2787 but now the leak is "stabilizing" at the last iteration: on master I get: |
|
@jnothman does the script run without leaking on your box? I ran it on OSX with a Python 2.7 after a BTW, maybe we could use |
|
Actually we should stick to It might be possible to select the right malloc at compile time with a preprocessor macro in our own |
This is going to add a lot of complexity for little gains, I believe. |
Well it would give us very fine control over memory management by making it possible to use the tracemalloc tool that supports differential snapshots to spot leaks along with the traceback of the faulty allocation. |
I agree. Let's not make this more complicated than it has already become. |
|
@ogrisel This is really odd. Here are the results on my box: |
| return | ||
| elif ret == 1: | ||
| raise RuntimeError('Cannot resize tree after building is complete') | ||
| raise MemoryError() |
There was a problem hiding this comment.
Couldn't you simply check self.locked here and not change the semantics of _resize_c?
There was a problem hiding this comment.
Anyway, can you comment on why we need this at all? This is internal API and the case this additional test covers should never happen. There is no need to be defensive in my opinion.
|
I re-pulled the |
|
I got the following results. |
|
It looks like that tree from 0.14.1 consumes less memory than master. :( |
|
Ok so @arjoly you have increasing yet stabilizing memory usage on 0.14.1 as well. So the leak might be fixed in this branch in the end. Python might be refusing to free some RSS for some other reason. |
I wasn't synced with master :-( |
|
Here is a new version of my script that tracks leaking objects (without references): import gc
import os
import psutil
import objgraph
import numpy as np
from sklearn.tree import ExtraTreeRegressor
X = np.random.normal(size=(100, 50))
Y = np.random.normal(size=(100, int(5e4)))
p = psutil.Process(os.getpid())
initially_without_ref = objgraph.get_leaking_objects()
def print_mem():
print("{:.0f}MB".format(p.get_memory_info().rss / 1e6))
currently_without_ref = objgraph.get_leaking_objects()
print([o for o in currently_without_ref
if o not in initially_without_ref])
print_mem()
for i in range(3):
et = ExtraTreeRegressor(max_features=1).fit(X, Y)
del et
gc.collect()
print_mem()Now the results:
I don't know what
So in the end this branch seems to correctly fix the regression introduced in master. |
|
Could someone explain me why we can't do as before and wrapped the c array into a numpy array at the last moment? |
|
@ogrisel Thanks for the check. So in conclusions, this PR correctly fixes the issue. |
| self.nodes = NULL | ||
| self.locked = False |
There was a problem hiding this comment.
This new attribute would deserve an inline comment to explain the motivation and how it is meant to be used.
There was a problem hiding this comment.
I would remove it (see my comment above).
Yes but before merging I think this PR should at least have more inline comments to explain the reference counting magic that happens under the hood. |
|
|
||
| # XXX using (size_t)(-1) is ugly, but SIZE_MAX is not available in C89 | ||
| # (i.e., older MSVC). | ||
| cdef int _resize_c(self, SIZE_t capacity=<SIZE_t>(-1)) nogil: | ||
| """Guts of _resize. Returns 0 for success, -1 for error.""" | ||
| if self.locked: | ||
| return 1 |
There was a problem hiding this comment.
if 0 is success and -1 is error -- what is 1?
There was a problem hiding this comment.
I'm happy to get rid of locked, but will leave a note in there for anyone
who tries to add post-building tree expansion. Comments coming.
On 25 January 2014 02:17, Peter Prettenhofer notifications@github.comwrote:
In sklearn/tree/_tree.pyx:
# XXX using (size_t)(-1) is ugly, but SIZE_MAX is not available in C89 # (i.e., older MSVC). cdef int _resize_c(self, SIZE_t capacity=<SIZE_t>(-1)) nogil: """Guts of _resize. Returns 0 for success, -1 for error."""
if self.locked:return 1if 0 is success and -1 is error -- what is 1?
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2790/files#r9150939
.
|
Awesome, thanks for your work Joel! I am merging this. |
[MRG] Avoid reference cycles in Tree
|
Great! Let's hope we've ironed out everything :) On the topic of which, implementing Tree.predict without _get_value_ndarray On 26 January 2014 00:15, Gilles Louppe notifications@github.com wrote:
|
|
Sorry for not being in time for a review, but this looks like an elegant solution. Thanks! |
|
Thanks for the fix !!! |
|
Thanks for the fix @jnothman. |
Fixes #2787.
This is a WIP becausewe need to check if wrappingvaluewith a ndarray at predict time is too expensive in time. If so, we can implement our owntakeusingmemcpy(which I'd rather over reverting to the version wherepredictduplicatesapply) but for the moment I'm having trouble getting that to work...This also needs to be tested for any further memory leaks.