BUG: break reference cycle in Index._engine
State: closed

- [X] closes #27585
- [X] tests added / passed
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [X] whatsnew entry


Labels:
name: Performance
description: Memory or execution speed performance

Comments:
@jreback I don't really understand what you mean. The issue is not the cache but that `self._engine_type` (e.g. `Int64Engine`) holds a callable that refers `self` (i.e. the `Index` instance). Putting the result of `self._engine_type(...)` into a weakref will clear it to quickly and won't cache it. I could use a callable for `self._engine_type` that uses a weakref to `self` instead of the `partial` solution that I am using now, but I don't see how this relates to `@cache_readonly`. Can you please be more specific about your suggestion? Thanks in advance.
> @jreback I don't really understand what you mean. The issue is not the cache but that `self._engine_type` (e.g. `Int64Engine`) holds a callable that refers `self` (i.e. the `Index` instance). Putting the result of `self._engine_type(...)` into a weakref will clear it to quickly and won't cache it. I could use a callable for `self._engine_type` that uses a weakref to `self` instead of the `partial` solution that I am using now, but I don't see how this relates to `@cache_readonly`. Can you please be more specific about your suggestion? Thanks in advance.

and I am not really sure what you suggested actually does anything. so let's revisit. what is the actual problem? having a cyclic reference in not a bug.
> and I am not really sure what you suggested actually does anything. so let's revisit. what is the actual problem? having a cyclic reference in not a bug.

You're right. I should have labeled it as a performance optimization. The issue with the reference cycle is that (similar to the one we had some time ago with the cycle between the indexer and the dataframe) that indices are not cleared from memory w/o running the GC. Under notebook / low-load conditions, this might not be an issue. For high-load system like dask / dask.distributed, this can be an issue since the python interpreter requires more memory than it should (peak memory consumption). I have actually observed exactly this issue under dask.distributed and depending on the load pattern, users can expect a temporary (i.e. until the GC finds the Index) over a gigabyte per worker. So what I would like to archive with this PR is that the index does not have a reference cycle and is cleared instantly when not used anymore.

If we agree on that change, I'll change the changelog entry and commit message before merging to make sure this is not labeled as a bug but as an enhancement. 
> > and I am not really sure what you suggested actually does anything. so let's revisit. what is the actual problem? having a cyclic reference in not a bug.
> 
> You're right. I should have labeled it as a performance optimization. The issue with the reference cycle is that (similar to the one we had some time ago with the cycle between the indexer and the dataframe) that indices are not cleared from memory w/o running the GC. Under notebook / low-load conditions, this might not be an issue. For high-load system like dask / dask.distributed, this can be an issue since the python interpreter requires more memory than it should (peak memory consumption). I have actually observed exactly this issue under dask.distributed and depending on the load pattern, users can expect a temporary (i.e. until the GC finds the Index) over a gigabyte per worker. So what I would like to archive with this PR is that the index does not have a reference cycle and is cleared instantly when not used anymore.
> 
> If we agree on that change, I'll change the changelog entry and commit message before merging to make sure this is not labeled as a bug but as an enhancement.

not objecting to the change in principle. But not sure how adding an indirection different from the lambda helps here.
> But not sure how adding an indirection different from the lambda helps here.

I've changed it to a more elegant solution and added a comment why this is helpful.
can you add an asv for this as well (the memory kind)
> can you add an asv for this as well (the memory kind)

Did so. Not sure if this is the right place, but it demonstrates the issue as well:

Before (current master):
```
$ asv run -e -E existing --bench gc
· Discovering benchmarks
· Running 1 total benchmarks (1 commits * 1 environments * 1 benchmarks)
[  0.00%] ·· Benchmarking existing-py_...
[100.00%] ··· index_object.GC.peakmem_gc_instances
[100.00%] ··· ======== ======
               param1
              -------- ------
                 1      125M
                 2      131M
                 5      155M
              ======== ======
```

After (this PR):
```
$ asv run -e -E existing --bench gc
· Discovering benchmarks
· Running 1 total benchmarks (1 commits * 1 environments * 1 benchmarks)
[  0.00%] ·· Benchmarking existing-py_...
[100.00%] ··· index_object.GC.peakmem_gc_instances
[100.00%] ··· ======== ======
               param1
              -------- ------
                 1      125M
                 2      125M
                 5      125M
              ======== ======
```
lgtm. you have some lint issues I think. ping on green.
@jreback ping :)
Thanks @crepererum!