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

Use PyType_Spec to define types instead of PyTypeObject. #187

Closed
wants to merge 19 commits into from

Conversation

glandium
Copy link
Contributor

This is a first step towards possibly using the stable ABI.

Copy link
Owner

@indygreg indygreg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing this. Given the size of the diff, I'm not sure when I'll get around to reviewing this. But it is a patch I'm tentatively interested in taking.

Regarding adoption of the stable ABI, I'm not opposed. But I thought we made use of unstable APIs (like maybe _PyBytes_Resize()?). Some of these might be necessary to achieve peak performance.

But out of principle I support converging onto the stable ABI where it doesn't introduce performance or other usability drawbacks.

@glandium
Copy link
Contributor Author

Would it help if I split this change in pieces, one per type?

The only two things that (besides Py_buffer, which is in the stable API since 3.11) are not in the stable API are _PyBytes_Resize and Py_SET_SIZE. When I looked superficially, it seemed it might be possible to switch to bytearrays.

@indygreg
Copy link
Owner

Would it help if I split this change in pieces, one per type?

Yes.

The only two things that (besides Py_buffer, which is in the stable API since 3.11) are not in the stable API are _PyBytes_Resize and Py_SET_SIZE. When I looked superficially, it seemed it might be possible to switch to bytearrays.

In theory we could switch to bytearray. But I consider this an API change since it is a distinct type from bytes. I'll need to think about the implications. I want to say I support the backwards compatibility break - it's for a good cause.

I also recall there being issues in older versions of Python where bytearray wasn't exactly as interchangeable with bytes as it should be. My memory is fuzzy. Maybe it was only on versions of Python we no longer support? But I do recall wanting to use bytearray at some point and something discouraged me from doing it. Maybe I left a breadcrumb somewhere...

@indygreg
Copy link
Owner

I'm going to ship the next release with zstd 1.5.4 to get that out the door. Then I'm going to drop Python 3.6 support. Then I'll look at this PR. And possibly start chipping at moving to the stable API so we can ship fewer wheels.

Copy link
Owner

@indygreg indygreg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cherry picked all these commits locally. Although I applied them out-of-order because I deferred the tp_buffer and version sniffing changes to the end because they were special and I didn't understand what was going on at first.

I do wonder why CPython didn't stabilize these at the same time as all the other slots. 🤷‍♂️

Will push to main once branch CI passes.

Thanks for this refactor! Greatly simplifies the definition of the C types!

@indygreg indygreg closed this in 7f0c3aa Feb 21, 2023
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