Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Support python 2. #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ENH: Support python 2. #7

wants to merge 1 commit into from

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.

2 participants