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

- [x] closes #25893
- [ ] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] 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 `cdef`ed in the calling function, that memory never gets released.  This is not an issue unique to pandas, as evidenced by [this stackflow question](https://stackoverflow.com/questions/29247373/memory-leak-calling-cython-function-with-large-numpy-array-parameters).  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](https://cython.readthedocs.io/en/latest/src/userguide/memoryviews.html#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.

Labels:
name: Performance
description: Memory or execution speed performance
name: Window
description: rolling, ewma, expanding

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=h1) Report
> Merging [#25926](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/1d4c89f4d747854def6c32733383b4afa250165d?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25926/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=tree)

```diff
@@            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% <ø> (ø)` | :arrow_up: |
| #single | `41.88% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `75% <0%> (-12.5%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.79% <0%> (-0.12%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.63% <0%> (-0.11%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=footer). Last update [1d4c89f...f055be9](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=h1) Report
> Merging [#25926](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f0ba498f71a9aed5b9cfda6ad6099fe9e7292cda?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25926/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=tree)

```diff
@@            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% <ø> (ø)` | :arrow_up: |
| #single | `41.89% <ø> (-0.08%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `75% <0%> (-12.5%)` | :arrow_down: |
| [pandas/core/groupby/grouper.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGVyLnB5) | `98.16% <0%> (-0.36%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.79% <0%> (-0.12%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `93.54% <0%> (ø)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.73% <0%> (+0.1%)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25926/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.79% <0%> (+0.21%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=footer). Last update [f0ba498...1d9f93a](https://codecov.io/gh/pandas-dev/pandas/pull/25926?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Can we add a memory ASV to measure this?

Sent from my iPhone

> On Mar 29, 2019, at 4:19 PM, codecov[bot] <notifications@github.com> 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.

cc @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
not necessarily part of this PR, but this file uses out-of-date no-python for-loop syntax, can be updated.
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.
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 `cdef`d 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.
thanks @ArtificialQualia 
numpy/numpy#13237 opened