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

[C API] Add PySet_NextEntry() function #121125

Closed
vstinner opened this issue Jun 28, 2024 · 12 comments
Closed

[C API] Add PySet_NextEntry() function #121125

vstinner opened this issue Jun 28, 2024 · 12 comments
Labels
topic-C-API type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

vstinner commented Jun 28, 2024

Feature or enhancement

The _PySet_NextEntry() function was removed in Python 3.13 alpha 1, but it's used by the Blender project: https://projects.blender.org/blender/blender/search?q=_PySet_NextEntry

I propose adding a new function to replace it:

int PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key)
  • Set *key to a strong reference, and return 1 on success.
  • Return 0 if there is no more item.
  • Set an exception and return -1 on error.

Differences with _PySet_NextEntry():

  • key is set to a strong reference, rather than a borrowed reference.
  • Remove the hash parameter.
@vstinner vstinner added type-feature A feature request or enhancement topic-C-API pending The issue will be closed if no feedback is provided labels Jun 28, 2024
@vstinner
Copy link
Member Author

cc @AdamWill

@erlend-aasland
Copy link
Contributor

Meta: why the pending label? Are you considering immediately closing your newly opened issue? :)

@vstinner vstinner removed the pending The issue will be closed if no feedback is provided label Jun 28, 2024
@vstinner
Copy link
Member Author

I added "pending" label by mistake, I don't know how. Removed.

@encukou
Copy link
Member

encukou commented Jun 28, 2024

Why is this better than PyObject_GetIter & PyIter_Next?

@vstinner
Copy link
Member Author

Why is this better than PyObject_GetIter & PyIter_Next?

I'm ok if we decide to not add a new function for Blender.

Proposed API is similar to PyDict_Next() with a pos parameter set to 0 at start. It's simpler to use, there is no iterator to manage. I also expect it to be faster.

@AdamWill
Copy link
Contributor

AdamWill commented Jun 28, 2024

Poking around, it's also used in Numba. PyO3 (Rust bindings for Python) used to use it but changed that recently in PyO3/pyo3#3849 . llvm-opt-benchmark seems to use it, but I think that's just benchmarking how fast it is when you build CPython with LLVM or something; presumably they could just...stop benchmarking that specific function. Everything else I can find at least in github universal search is some kind of fork or copy of CPython or one of the other users, or an alternative Python interpreter providing its own version.

@serhiy-storchaka
Copy link
Member

Do you want to add only PySet_NextEntry(), or also PyFrozenSet_NextEntry() and PyAnySet_NextEntry()?

See #43090 for discussion about PySet_Next().

cc @rhettinger

@vstinner
Copy link
Member Author

I expect the function to work on set, frozenset and subclasses.

@rhettinger
Copy link
Contributor

I prefer people to just use __iter__ and __next__. Those are reasonably fast, are standard, are well-understood, and have strong guarantees around them (something that will become even more important in free-threaded code).

The _PySet_NextEntry API is fragile, hard to use correctly, and too tightly coupled to implementation details. It was intended solely an internal optimization hack. The warning comment for set_next is dead serious. Even for internal calls, we've had bugs related to its use.

@rhettinger
Copy link
Contributor

FWIW, it was an intentional decision to not make this a public API.

The API was carefully thought out to provide what most people need from a set object. Implementation details, optimization hacks, and fragile interfaces were avoided. For the most part, this has served us well.

@rhettinger rhettinger closed this as not planned Won't fix, can't repro, duplicate, stale Jul 5, 2024
@vstinner
Copy link
Member Author

vstinner commented Jul 8, 2024

@AdamWill: Well, it seems like Blender, other mentioned projects, have to be updated to PyObject_GetIter() and PyIter_Next().

@AdamWill
Copy link
Contributor

AdamWill commented Jul 9, 2024

Thanks, I'll update the downstream issue. If anyone wants to help with that it would be great, I like to try and help projects with these transitions where possible but I'm not much of a C coder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

6 participants