Skip to content

Conversation

@ssanderson
Copy link

Initial cut of support for python 2.

Notable deficiencies:

  • Removes keyword-only args from several API functions.

  • Changes BytesIO.getbuffer() to BytesIO.getvalue(). This is almost
    certainly a significant performance regression for large loads. At the
    very least, it should be made conditional by python version. It's an
    open question whether the extra copy incurred by calling getvalue()
    makes this even worth doing. If not, we'd probably have to vendor a
    py2-compat BytesIO-like object to support zero-copy reads.

Initial cut of support for python 2.

Notable deficiencies:

- Removes keyword-only args from several API functions.

- Changes `BytesIO.getbuffer()` to `BytesIO.getvalue()`. This is almost
  certainly a significant performance regression for large loads. At the
  very least, it should be made conditional by python version.  It's an
  open question whether the extra copy incurred by calling `getvalue()`
  makes this even worth doing. If not, we'd probably have to vendor a
  py2-compat BytesIO-like object to support zero-copy reads.
@ssanderson ssanderson requested a review from llllllllll May 1, 2017 13:50
@ssanderson
Copy link
Author

Not sure if this is actually worth doing, but, I was seeing unreasonable memory usage for fundamentals queries on Q over the weekend, and wanted to at least test to see if warp_prism improved that situation.

@llllllllll
Copy link
Contributor

In Python 2 cStringIO has a C API where you can read from the underlying buffer.

There is a capsule called cStringIO_CAPI which exports a structure with a read function. You can pass -1 to read the whole thing as a char*. This doesn't copy the data. This can't be merged as is because of the getvalue call.

Even with the getvalue call was this better for fundies?

@ssanderson
Copy link
Author

Even with the getvalue call was this better for fundies?

Don't know, I haven't actually used this yet other than getting the tests to pass in py2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants