BUG: Fix memory issues in rolling.min/max
State: closed

This fixes at least the reproducible part of #30726. I am not
totally sure what is going on here (is this a true memory leak?), and whether this fixes all issues, but it does strongly reduce memory usage as measured by `psutil`.

My tests have shown that there are two solutions that avoid growing memory
usage:

- pass memoryviews (`float64_t[:]`) instead of `ndarray[float64_t]`
- remove `starti` and `endi` as arguments to `_roll_min_max_fixed`

This commit implements both, since `_roll_min_max_fixed` doesn't use `starti` and `endi` anyways.

- [x] fixes at least part of 
closes #30726 
closes #32466
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


I am unsure about testing: Does this need a test? If so, what would be a good way to go about testing? The tests I performed (example code in the linked issue) are probably very system specific.

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

Comments:
> I am unsure about testing: Does this need a test? If so, what would be a good way to go about testing? The tests I performed (example code in the linked issue) are probably very system specific.

Definitely needs a test.  How about you implement a test based on the example in #30726 (though pls construct inputs directly, dont use skimage/scipy) and we'll see how the CI reacts.
Upon closer inspection of  #25926, I found that this fixes exactly the same issue. It seems to have been reintroduced at some point. An asv benchmark for this already exists.
> > I am unsure about testing: Does this need a test? If so, what would be a good way to go about testing? The tests I performed (example code in the linked issue) are probably very system specific.
> 
> Definitely needs a test. How about you implement a test based on the example in #30726 (though pls construct inputs directly, dont use skimage/scipy) and we'll see how the CI reacts.

How would I go about measuring the memory use? As shown in the issue, I used `psutil` to show system memory usage. During testing, I made sure to not have other processes running that could lead to larger changes in memory usage. I'm not sure that will work with CI.
you can add memory benchmarks with asv

we have a few,
search for _mem_ i think
> you can add memory benchmarks with asv
> 
> we have a few,
> search for _mem_ i think

I changed the numbers for the asv peakmem benchmark from #25926 so that the isse is a bit more visible. Should I still add a test for the pytest test suite, or is the benchmark enough?
> > you can add memory benchmarks with asv
> > we have a few,
> > search for _mem_ i think
> 
> I changed the numbers for the asv peakmem benchmark from #25926 so that the isse is a bit more visible. Should I still add a test for the pytest test suite, or is the benchmark enough?

no test needed, but see my comments above
pending asv results this lgtm
Here are the results of the asv run for master:
```
 $ asv show master
Commit: master <master>

rolling.PeakMemFixedWindowMinMax.peakmem_fixed [my_machine/virtualenv-py3.8-Cython-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pytest-scipy-sqlalchemy-tables-xlrd-xlsxwriter-xlwt]
  ok
  ======================== =======
           param1                 
  ------------------------ -------
   <function Rolling.min>   1.72G 
   <function Rolling.max>   1.72G 
  ======================== =======
  started: 2020-04-26 11:28:00, duration: 19.8s
```

And this is on the new branch:
```
 $ asv show rolling-min-max-memory-leak
Commit: rolling- <rolling-min-max-memory-leak>

rolling.PeakMemFixedWindowMinMax.peakmem_fixed [my_machine/virtualenv-py3.8-Cython-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pytest-scipy-sqlalchemy-tables-xlrd-xlsxwriter-xlwt]
  ok
  ======================== ======
           param1                
  ------------------------ ------
   <function Rolling.min>   118M 
   <function Rolling.max>   118M 
  ======================== ======
  started: 2020-04-26 11:06:59, duration: 22.6s
```
Hello @s-scherrer! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2020-04-28 09:35:51 UTC
This fix also resolves #32266 - which seems to be a duplicate of the original issue here too.

Considering that this is a pretty severe bug in the whole 1.0.x tree, i think this should be part of the 1.0.4 milestone, too.
Here are the new results for master:
```
 $ asv show master
Commit: master <master>

rolling.PeakMemFixedWindowMinMax.peakmem_fixed [my_machine/virtualenv-py3.8-Cython-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pytest-scipy-sqlalchemy-tables-xlrd-xlsxwriter-xlwt]
  ok
  ======================== ======
           param1                
  ------------------------ ------
   <function Rolling.min>   262M 
   <function Rolling.max>   262M 
  ======================== ======
  started: 2020-04-26 15:09:36, duration: 1.12s
```

and on the new branch:
```
 $ asv show rolling-min-max-memory-leak
Commit: rolling- <rolling-min-max-memory-leak>

rolling.PeakMemFixedWindowMinMax.peakmem_fixed [my_machine/virtualenv-py3.8-Cython-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pytest-scipy-sqlalchemy-tables-xlrd-xlsxwriter-xlwt]
  ok
  ======================== ======
           param1                
  ------------------------ ------
   <function Rolling.min>   198M 
   <function Rolling.max>   198M 
  ======================== ======
  started: 2020-04-26 15:08:56, duration: 1.32s
```

It's not below 1s, but close. I'm also running this on a pretty old thinkpad with a pre-2010 Core 2 Duo processor, so I guess it will run faster on newer machines.
can you add a longer test to be sure it does not affect performance of these widely used methods?
thanks @s-scherrer very nice
@jreback any chance of backporting this into the next 1.0.x release, or is it too late for that as it's due in a few days?

I'd think that would be beneficial as it's fixing a nasty bug... 
@xmatthias this is a plan for a 1.0.4 release, I believe @simonjayhawkins is planning on this. But as pandas is fully volunteer it may or may not happen.
Don't misunderstand me - i'm aware that pandas is fully volunteer (and i appreciate and thank anyone spending their time on it!) -

i'm just trying to say that it would be great to have this bugfix included in the next possible release.
> Don't misunderstand me - i'm aware that pandas is fully volunteer (and i appreciate and thank anyone spending their time on it!) -
> 
> i'm just trying to say that it would be great to have this bugfix included in the next possible release.

yes if we have a 1.04 it will be included, its also in 1.1, which actually should be soon anyhow