PERF: Improve pickle support with BZ2 & LZMA
State: closed

- [x] Fixes #46747
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Performance
description: Memory or execution speed performance
name: IO Pickle
description: read_pickle, to_pickle

Comments:
Noting that we're having unrelated, CI issues currently. Hopefully it should be fixed today.
No worries. Thanks Matthew! 🙏

Looks like any related test failures have been fixed, but will recheck once CI issues clear.

If you have thoughts on this approach, please let me know. Though no worries if you are busy 🙂
The approach seems reasonable; it would be good to ensure we have tests to ensure the `raw` and `memoryview` path are both tested.

Also am I correct in understanding in https://github.com/pandas-dev/pandas/issues/46747 that once we drop 3.9 that this won't be entirely necessary or would it still be a good performance benefit to keep?
Mostly, [Python 3.9.6 has the fix]( https://docs.python.org/3.9/whatsnew/changelog.html?highlight=bpo-44439#python-3-9-6-final ). Also the fix is included in [Python 3.10.0b4]( https://docs.python.org/3.10/whatsnew/changelog.html?highlight=bpo-44439#python-3-10-0-beta-4 ) and [Python 3.11.0a1]( https://docs.python.org/3.11/whatsnew/changelog.html?highlight=bpo-44439#python-3-11-0-alpha-1 ). So Python 3.9.6+ don't need the workaround. Python 3.8 and earlier versions of Python 3.9 would need the workaround. IOW it is probably safer to keep until Python 3.10 is the minimum (or when Python 3.9 is dropped).

Once Python 3.10 is minimum, we could drop the wrapper classes and just keep the simplified `to_pickle`.

Happy to flag the code based on Python version somehow if that makes it easier to track.

In terms of testing, think most of what we need is covered in [these tests]( https://github.com/pandas-dev/pandas/blob/864729813a0203af8bb0d30b6c883588ae2c96f8/pandas/tests/io/test_pickle.py#L256 ). The default path `raw` should be handled there (though please correct me if I'm missing something). The `memoryview` case would only come up if the data was not contiguous somehow (for example `numpy.arange(10)[::2]`). Do you know of good examples in Pandas where that would come up?
> Happy to flag the code based on Python version somehow if that makes it easier to track.

That would be really helpful as cleaning up code is always nice :)

> The memoryview case would only come up if the data was not contiguous somehow (for example numpy.arange(10)[::2]). Do you know of good examples in Pandas where that would come up?

Yeah this can come up in pandas with ExtensionArrays (1D) being indexed in a DataFrame (2D). We have some tests checking contiguousness of the underlying numpy array when indexing https://github.com/pandas-dev/pandas/blob/2e7f5a30abdfed4f34902ab1effee65e9fc48b4e/pandas/tests/extension/base/dim2.py#L274
Sounds good. Have refactored out a common utility function, `flatten_buffer`, which simplifies both subclasses. Also `flatten_buffer` is tested in a variety of cases to make sure it is coercing data into something both compressors can handle (without the fix). Also conditioned the subclass patches on Python version support to clarify things. The subclasses themselves have been moved into `compat`.
Consolidated the compat changes into a single file. Also refactored `PY310` and other platform constants into a single file used here. Added more types. Also added a few more test cases. Please let me know what you think 🙂
Tried to use the request a review feature, but it seems to not allow requesting both reviewers 🤷‍♂️
Explicitly listed all constants.

Have not added a news entry yet. Please let me know where we should add this.

Idk if 1.x is still being developed, but it may make sense to backport there as well if it is.
> Have not added a news entry yet. Please let me know where we should add this.
> 
> Idk if 1.x is still being developed, but it may make sense to backport there as well if it is.

Should probably go in 2.0.0 under "performance improvements" or in "enhancements"
Anything else needed here?
How does the updated changelog entry look?
Sweet thanks @jakirkham! 
Awesome! Thank you both 😄

Would it be possible to backport this change as well? If so, what is needed from me to do that?
Backports are currently done for regressions (bug/performance) that were introduced in 1.5. IIUC the performance of `to_pickle` in this situation didn't necessarily regress in 1.5?
Good question. The history goes like this:

* PR ( https://github.com/pandas-dev/pandas/pull/37056 ) enabled this for all compressors in 1.2
* Users discovered issues with BZ2 & LZMA ( https://github.com/pandas-dev/pandas/issues/39002 )
* PR ( https://github.com/pandas-dev/pandas/pull/39376 ) special cased BZ2 & LZMA to fix the bug in 1.2.2 (though this has reduced perf, which of course is the right tradeoff)

So the perf improvement and then reduction occurred in 1.2 (not 1.5).

That said, maybe there is still value in having this perf improvement in some 1.x version (especially given the move to 2 occurring). What do you think? 🙂

Edit: Should add have no strong feelings here. Just wondering if we can help 1.x users with this change.
Would be a nice performance benefit for 1.5.x users. Also need to weigh any unknown bugs/behavior changes this might introduce. Thoughts @pandas-dev/pandas-core? 
I would err on the side of caution and go 2.0
I'd also prefer to keep this in on main only