BUG: Fix memory leak in Rolling.min and Rolling.max (#25893)

[ArtificialQualia]

• closes Memory leak with .rolling().max() in pandas 0.24.2  #25893
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

This fixes an issue that caused a memory leak in window._roll_min_max. Apparently, when you pass an ndarray as an argument that you cdefed in the calling function, that memory never gets released. This is not an issue unique to pandas, as evidenced by this stackflow question. It may be a numpy bug.

There are a few potential fixes for this:

• Moving everything into one function
• Pass pointers to the functions
• Use memoryviews

As memoryviews is the least disruptive change and is just as performant as what was already being used, that's what I went with for this fix.

I did not add a test with this PR, as determining memory usage is platform specific (and rather wonky) unless you use a library like psutils, and even with that may have problems.

[codecov]

Codecov Report

Merging #25926 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25926      +/-   ##
==========================================
- Coverage    91.8%   91.79%   -0.01%     
==========================================
  Files         174      174              
  Lines       52536    52536              
==========================================
- Hits        48231    48226       -5     
- Misses       4305     4310       +5

Flag	Coverage Δ
#multiple	90.35% <ø> (ø)	⬆️
#single	41.88% <ø> (-0.07%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	75% <0%> (-12.5%)	⬇️
pandas/core/frame.py	96.79% <0%> (-0.12%)	⬇️
pandas/util/testing.py	90.63% <0%> (-0.11%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d4c89f...f055be9. Read the comment docs.

[codecov]

Codecov Report

Merging #25926 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25926      +/-   ##
==========================================
+ Coverage   91.81%   91.81%   +<.01%     
==========================================
  Files         175      175              
  Lines       52578    52580       +2     
==========================================
+ Hits        48273    48275       +2     
  Misses       4305     4305

Flag	Coverage Δ
#multiple	90.36% <ø> (ø)	⬆️
#single	41.89% <ø> (-0.08%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	75% <0%> (-12.5%)	⬇️
pandas/core/groupby/grouper.py	98.16% <0%> (-0.36%)	⬇️
pandas/core/frame.py	96.79% <0%> (-0.12%)	⬇️
pandas/core/generic.py	93.54% <0%> (ø)	⬆️
pandas/util/testing.py	90.73% <0%> (+0.1%)	⬆️
pandas/core/indexes/base.py	96.79% <0%> (+0.21%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0ba498...1d9f93a. Read the comment docs.

[WillAyd]

Can we add a memory ASV to measure this?

…

Sent from my iPhone

On Mar 29, 2019, at 4:19 PM, codecov[bot] ***@***.***> wrote: Codecov Report Merging #25926 into master will decrease coverage by <.01%. The diff coverage is n/a. @@ Coverage Diff @@ ## master #25926 +/- ## ========================================== - Coverage 91.8% 91.79% -0.01% ========================================== Files 174 174 Lines 52536 52536 ========================================== - Hits 48231 48226 -5 - Misses 4305 4310 +5 Flag Coverage Δ #multiple 90.35% <ø> (ø) ⬆️ #single 41.88% <ø> (-0.07%) ⬇️ Impacted Files Coverage Δ pandas/io/gbq.py 75% <0%> (-12.5%) ⬇️ pandas/core/frame.py 96.79% <0%> (-0.12%) ⬇️ pandas/util/testing.py 90.63% <0%> (-0.11%) ⬇️ Continue to review full report at Codecov. Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1d4c89f...f055be9. Read the comment docs. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[WillAyd]

cc @jbrockmendel

[jbrockmendel]

This looks harmless and I'm happy to take your word for it that it fixes the referenced issue. A couple of small requests commented inline

[jbrockmendel]

not necessarily part of this PR, but this file uses out-of-date no-python for-loop syntax, can be updated.

[jreback]

this needs a memory asv. can you show a %memit before and after this PR as well.

[ArtificialQualia]

Here are the %memit results:

Function definition:

def series_mem():
    for x in range(100000):
        pd.Series(data=np.random.rand(5000)).rolling(4000).max()

Master:

peak memory: 165.94 MiB, increment: 87.94 MiB

PR:

peak memory: 82.07 MiB, increment: -0.03 MiB

I'll work on adding an asv test for this.

[ArtificialQualia]

So, while writing the ASV tests, I learned that this bug doesn't actually affect _roll_min_max_variable. The only reason I can guess is that starti and endi are actually used in that function, and this bug only happens when ndarray as an arg that was previously cdefd that you don't use. Should have caught that earlier.

But that makes the fix even easier! Since they aren't used, just remove them, no changing anything to memoryviews at all.

Anyway, here are the ASV results:

       before           after         ratio
     [1d4c89f4]       [d4518014]
     <master>         <fix-memory-leak>
-         70.3M            58.5M       0.83  rolling.PeakMemFixed.peakmem_fixed('max')
-         70.5M            58.2M       0.83  rolling.PeakMemFixed.peakmem_fixed('min')

Some notes on the ASV tests are in the code PR comments.

[jreback]

thanks @ArtificialQualia

[ArtificialQualia]

numpy/numpy#13237 opened