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

Cython calls PySequence_GetSlice instead of PyObject_GetItem #1195

Closed
robertwb opened this issue Dec 30, 2010 · 5 comments
Closed

Cython calls PySequence_GetSlice instead of PyObject_GetItem #1195

robertwb opened this issue Dec 30, 2010 · 5 comments

Comments

@robertwb
Copy link
Contributor

In Python, __getitem___ has been preferred over __getslice__ for ages and __getslice__ is gone for good in Python 3. However, Cython still generates slicing code instead of item getting code for performance reasons. It hard codes the slice sizes and calls PySequence_GetSlice() with Py_ssize_t arguments. This leads to a difference to Python when creating the slice object.

In Python, obj[:] calls the object's __getitem__ method with slice(None,None,None) as argument. In Cython, the slice is created as slice(0,PY_SSIZE_T_MAX,None).

At least in Python 3, where PySequence_GetSlice() always creates a slice object, this is worth fixing (and optimising by using a constant slice object for constant cases). In Python 2, the slice object is only created when the class defines __getitem__ and not __getslice__, which makes it trickier to fix.

Note that the same applies to SetItem and DelItem.

Also note that it is not enough to translate any Py_SSIZE_T_MAX into None, as the user may well have deliberately used this value.

First reported for Debian here:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=604963

Migrated from http://trac.cython.org/ticket/636

@robertwb
Copy link
Contributor Author

scoder commented

Compared to Python 2.7, the ListSlicing test in pybench runs slower:

Test           minimum run-time        average  run-time
               this    other   diff    this    other   diff
------------------------------------------------------------
ListSlicing:   696ms   647ms   +7.6%   788ms   673ms  +17.2%

This issue might be related.

@robertwb
Copy link
Contributor Author

scoder commented

Due to the way 2-value slicing is currently implemented, this fails:

l = [= l[None:None](1,2,3]
sl)

The slice indices are coerced to Py_ssize_t during type analysis, thus raising a type error at runtime.

@robertwb
Copy link
Contributor Author

scoder changed priority from major to critical
commented

Raising the priority as the impact of the current implementation is starting to add up.

@robertwb
Copy link
Contributor Author

scoder changed owner from somebody to scoder
status from new to assigned
commented

@robertwb
Copy link
Contributor Author

scoder changed milestone from wishlist to 0.19
resolution to fixed
status from assigned to closed
commented

Implemented here and in the subsequent commits:

scoder@306c3a7

Constant bounds also use a cached slice instance now, which should speed up slicing in Py3.

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

No branches or pull requests

1 participant