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

PyObjectAlloc no longer uses specialization and freelist is removed #264

Closed
wants to merge 1 commit into from
Closed

PyObjectAlloc no longer uses specialization and freelist is removed #264

wants to merge 1 commit into from

Conversation

ijl
Copy link
Contributor

@ijl ijl commented Nov 11, 2018

The impl<T> PyObjectAlloc for PyObjectWithFreeList is an obstacle to
removing specialization. It is unused in all projects linked from
the README and within pyo3 itself, so let's just remove it.

@ijl
Copy link
Contributor Author

ijl commented Nov 11, 2018

cc #210

@konstin
Copy link
Member

konstin commented Nov 11, 2018

I have mixed feelings about this. It's true that it's an obstacle for compiling on stable and not widely used. OTOH it's a feature coming directly from the python c api and is relevant to performance. Would it be possible to feature gate with an off-by-default freelist feature?

CHANGELOG.md Outdated Show resolved Hide resolved
The `impl<T> PyObjectAlloc` for PyObjectWithFreeList is an obstacle to
removing specialization. It is unused in all projects linked from
the README and within pyo3 itself, so let's just remove it.
@ijl
Copy link
Contributor Author

ijl commented Nov 11, 2018

Having the feature only benefits custom classes that choose to define it, and I don't see any in pyo3 or client projects. CPython classes are unaffected. If there is zero usage I think that suggest removing it. But I can look into a feature if you still prefer keeping it.

@kngwyu
Copy link
Member

kngwyu commented Nov 12, 2018

I'm against this change.
To remove default here means we can't override alloc/dealloc and I don't think it's a correct approach.

@konstin
Copy link
Member

konstin commented Nov 12, 2018

@kngwyu default doesn't work on stable, so we need to remove it someway. It's still possible to support overwrite alloc by using macro arguments.

@fafhrd91 you added the freelist; Do you know projects using that feature?

@kngwyu
Copy link
Member

kngwyu commented Nov 12, 2018

@konstin
But we have a custom dealloc implementation here https://github.com/rust-numpy/rust-numpy/blob/master/src/slice_box.rs#L67
So how about moving PyObjectAlloc's methods into PyTypeInfo?
If so we can still add custom alloc/dealloc implementations for a type which implements PyTypeInfo.

@konstin
Copy link
Member

konstin commented Nov 12, 2018

What would you think about this branch? Afaik that should work for all cases and we can even allow overriding with #[pyclass] later.

@kngwyu
Copy link
Member

kngwyu commented Nov 12, 2018

@konstin
Strongly 👍 for it!
This looks very natural solution to me.

@ijl ijl closed this Nov 12, 2018
@ijl ijl deleted the spec-freelist branch November 12, 2018 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants