ENH: always make ndarrays from msgpack writable
State: closed

Addresses the case where 'compress' was not none. The old implementation
would decompress the data and then call np.frombuffer on a bytes
object. Because a bytes object is not a mutable buffer, the resulting
ndarray had writeable=False. The new implementation ensures that the
pandas is the only owner of this new buffer and then sets it to mutable
without copying it. This means that we don't need to do a copy of the
data coming in AND we can mutate it later. If we are not the only owner
of this data then we just copy it with np.fromstring.


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

Comments:
cc @kawochen 

python magic!  there might be performance warnings during debugging when the debugger holds more references

I think that warning while debugging is unfortunate but correct. Do you think that it would be worth amending the warning to add "or you are in a debugger"?

I'm not sure what the best thing to do is.  In general I believe relying on ref count is sketchy (e.g. count could be incremented between function calls).  But then I think `zlib` doesn't really return anything that's shared, even for small strings (I didn't manage to get any performance warnings with crafted cases).

I'm only minimally familiar with the C-API.  Is it possible that, although the ref count is 2, the string is interned during the query (maybe by `getrefcount` itself) and never destroyed, so subsequent attempts to use that string give unexpected results?

In moving data around, this `n` vs `2n` memory issue is very common, but I think it is a legit concern, and in this case it does seem a bit wasteful.

Subsequent accesses to the `bytes` object will have unexpected results. Think of this as a `move` operation. I have checked the source for zlib and blosc to confirm that in their current state they will not be interning the strings; however, the test cases are to guard against changes to the implementation of these libraries. I also have open PRs upstream to both CPython and blosc that will make these functions return mutable buffers. We will need to support this hack for some time (especially with zlib) but at least there will be a more supported code path. I have an idea for a function that will factor out this logic a bit and ensure that users cannot get a reference back to the invalid bytes object. Right now this object is still exposed as `df._data.blocks[n].values.base.base` which is not ideal but also pretty deeply hidden/

@kawochen I am updating the PR with a new implementation that I think is both safer and faster (or at least the same speed).

With this new implementation:

``` python
In [34]: pd.read_msgpack(df.to_msgpack(compress='blosc'))._data.blocks[0].values.base.base
Out[34]: <memory at 0x7f71f8e81750>
```

However with the old implementation the base was the `bytes` object that shared a buffer with the array.

Here are profiling runs to show the differences:

These cells are used for all three profiling runs.

`````` python
In [1]: df = pd.DataFrame({'a': np.arange(1000000), 'b': np.arange(1000000)})

In [2]: packed = df.to_msgpack(compress='blosc')

Using `np.fromstring` to copy the data (this is just shown to make it clear why we cannot do this):

```python
In [3]: %timeit pd.read_msgpack(packed)
10 loops, best of 3: 168 ms per loop
``````

Old implementation:

``` python
In [3]: %timeit pd.read_msgpack(packed)
100 loops, best of 3: 3.06 ms per loop
```

Updated implementation:

``` python
In [5]: %timeit pd.read_msgpack(packed)
100 loops, best of 3: 2.68 ms per loop
```

Ugh, I forgot that `memoryview` has an `obj` attribute which is exactly like `ndarray`s `base` attribute. I am investigating ways to drop this in pure python but I am starting to think that we might just need to use a little ctypes to kill this object.

This is on master (not  your branch)

```
In [23]: %timeit pd.read_pickle('test.pkl')
100 loops, best of 3: 13.7 ms per loop
```

```
# zlib
[22]: %timeit pd.read_msgpack('test.pak')
10 loops, best of 3: 144 ms per loop

# blosc
In [3]: %timeit pd.read_msgpack('test.pak')
100 loops, best of 3: 3.19 ms per loop

# uncompressed
In [5]: %timeit pd.read_msgpack('test.pak')
100 loops, best of 3: 18.5 ms per loop
```

blosc >> zlib

@llllllllll 

This is on 0.17.1

```
In [1]: df = pd.DataFrame({'a': np.arange(1000000), 'b': np.arange(1000000)})

In [2]: In [2]: packed = df.to_msgpack(compress='blosc')

In [3]: %timeit pd.read_msgpack(packed)
100 loops, best of 3: 4.97 ms per loop
```

How did you get the above?

I am amazed on how fast `blosc` is. we should turn this on by default if `blosc` is available.

sorry, the really slow run was what would happen if we copied the data after doing `blosc.decompress`. This code was never on master I just wanted to show that it was worthwhile to use a more complex approach to avoid this copy.

ping, how do you feel about this?

no trace of `bytes` left :+1: 

hype

what triggers the error on master?

which error?

the problem IIUC which you are solving is that you are getting a non writable buffer back right?
as there doesn't seem to be a perf issue with master

There is no perf error with master. I was just demonstrating the difference between a copying approach to get mutability and my non-copying approach. Master does not copy but is also does not give a mutable ndarray. The error would be if you just set `array.flags.writable = True` because then you could do `array.base` and get a `bytes` object which _should_ be immutable but now shares memory with a mutable array.

can u show me a case where we get an exception ? 

I get what u r doing - I am just want to see an example of the practical problem

I see u did an example way at the top
but I want to see s pandas operation that triggers the error
m

I am not sure when or if I will be able to get to rewriting this in Cython. If someone with more cython experience wants to take this over to help get this done I wouldn't mind and could answer any questions / clarify when the comments are not enough.

ping @jreback 

Updated based on feedback. I also added standalone tests for `move_into_mutable_buffer` because it is not in `util`.  

IIRC this DID work on windows before.

```
[pandas3] C:\Users\conda\Documents\pandas3.5>python setup.py build_ext --inplace
running build_ext
skipping 'pandas\io/sas\saslib.c' Cython extension (up-to-date)
skipping 'pandas\src\period.c' Cython extension (up-to-date)
skipping 'pandas\hashtable.c' Cython extension (up-to-date)
skipping 'pandas\algos.c' Cython extension (up-to-date)
skipping 'pandas\index.c' Cython extension (up-to-date)
skipping 'pandas\parser.c' Cython extension (up-to-date)
skipping 'pandas\lib.c' Cython extension (up-to-date)
skipping 'pandas\tslib.c' Cython extension (up-to-date)
skipping 'pandas\src\sparse.c' Cython extension (up-to-date)
skipping 'pandas\src\testing.c' Cython extension (up-to-date)
skipping 'pandas\msgpack\_packer.cpp' Cython extension (up-to-date)
skipping 'pandas\msgpack\_unpacker.cpp' Cython extension (up-to-date)
building 'pandas.util._move' extension
creating build\temp.win-amd64-3.5\Release\pandas\util
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\amd64\cl.exe /c /nologo /Ox /W3 /GL /DNDEBUG /MD -IC:\Miniconda2\envs\pandas3.5\lib\site-pa
ckages\numpy\core\include -IC:\Miniconda2\envs\pandas3.5\include -IC:\Miniconda2\envs\pandas3.5\include "-IC:\Program Files (x86)\Microsoft Visual Stu
dio 14.0\VC\INCLUDE" "-IC:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\ATLMFC\INCLUDE" "-IC:\Program Files (x86)\Windows Kits\10\include\10.0.
10240.0\ucrt" "-IC:\Program Files (x86)\Windows Kits\NETFXSDK\4.6\include\um" "-IC:\Program Files (x86)\Windows Kits\10\include\10.0.10240.0\shared" "
-IC:\Program Files (x86)\Windows Kits\10\include\10.0.10240.0\um" "-IC:\Program Files (x86)\Windows Kits\10\include\10.0.10240.0\winrt" /Tcpandas/util
/_move.c /Fobuild\temp.win-amd64-3.5\Release\pandas/util/_move.obj
_move.c
pandas/util/_move.c(201): error C2099: initializer is not a constant
error: command 'C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\BIN\\amd64\\cl.exe' failed with exit status 2

[pandas3] C:\Users\conda\Documents\pandas3.5>

```

That is saying that my definition for `stolenbuf_type` is not constant but it only uses addesses and int constants, is this using PIC? is there a way to get the compiler to emit the particular field that it does not think is constant? GCC by default will tell you which field it doesn't like.

https://msdn.microsoft.com/en-us/library/t801az8a.aspx

Read some docs about windows dev and pushed a potential fix. Things still behave normally for me with gcc on gnu+linux, hopefully this builds on windows. Do you have an appveyor build I can check?

you _could_ set this up with appveyor (just create an account), all of the setup will build on your PR's. but its VERY slow. I do with a local vm.

ok this passed for me on windows!

merging

side issue, should prob add some more perf tests with msgpack to cover the bases.