Skip to content

Conversation

jimkring
Copy link
Contributor

@jimkring jimkring commented Sep 6, 2023

here’s a suggestion from @colesbury:

This looks like it needs to use Py_REFCNT(obj) instead of obj->ob_refcnt. The reference count field changed (it's now two fields in "nogil Python), so you can't access them directly. The Python docs now say to use Py_REFCNT instead of ->ob_refcnt so maybe we can get this change upstreamed.

use Py_REFCNT(obj) instead of obj->ob_refcnt
@nascheme
Copy link

I think assuming refcnt == 1 means that it's safe to mutate it is not safe. It's not safe in both the free-threaded and the regular build. I don't know how much performance would be lost by disabling this. You might check that Python version is < 3.13 since the upcoming release will have more ways to break this assumption (was already broken in <= 3.12, I think).

There is a new API being proposed for CPython:
python/cpython#133170

@colesbury
Copy link

colesbury commented Apr 30, 2025

The Py_REFCNT(obj) == 1 check is fine here. There obj comes from a PyBytes_FromStringAndSize() call. There are only two cases:

  • PyBytes_FromStringAndSize() returns a new bytes object with refcount of 1. Nobody else has access to it, so it's safe to resize in-place.
  • PyBytes_FromStringAndSize() returns an interned, immortal bytes object. The refcount is not one (it's some large value) and we don't resize it.

There's no concern about racing accesses to the refcount here in the free threading build. It's a lot more straightforward than the NumPy because the PyBytesObject is coming from python-zstandard code, not from some Python call.

@indygreg
Copy link
Owner

I've cherry picked this and will push it directly. PR will be marked as closed but it is in fact merged.

@indygreg indygreg closed this in a83ac9f Aug 16, 2025
@jimkring
Copy link
Contributor Author

Thanks @indygreg! Appreciate your great work on this.

@indygreg
Copy link
Owner

I thought I saw this commit pass CI. But it definitely resulted in crashes after the merge to main. So I'm reverting it. https://github.com/indygreg/python-zstandard/actions/runs/17011229024

Some of the wheel build logs show a compiler error likely pointing at the source of the bug. https://github.com/indygreg/python-zstandard/actions/runs/17011229015/job/48227527781

indygreg added a commit that referenced this pull request Aug 16, 2025
indygreg pushed a commit that referenced this pull request Aug 23, 2025
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.

4 participants