GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe by corona10 · Pull Request #114843 · python/cpython Conversation
|
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Regarding iterators: list (and dict) iterators current clear it->it_seq field when the iterator is exhausted. This will make it harder to make the iterators thread-safe because it_seq is mutable. In the free-threaded build, I think we should avoid clearing the it_seq field until the iterator is deallocated.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| #ifdef Py_GIL_DISABLED | ||
| #define LOAD_SSIZE_ATOMIC_AS_POSSIBLE(value) _Py_atomic_load_ssize_relaxed(&value) | ||
| #define STORE_SSIZE_ATOMIC_AS_POSSIBLE(value, new_value) _Py_atomic_store_ssize_relaxed(&value, new_value) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The name is a bit confusing to me -- I'm not sure what AS_POSSIBLE means here. Maybe just LOAD_SSIZE(). It's not very descriptive, but at least it's short.
The style I've seen more commonly in CPython looks like:
#ifdef Py_GIL_DISABLED
# define LOAD_SSIZE_ATOMIC_AS_POSSIBLE(value) _Py_atomic_load_ssize_relaxed(&value)
#endif
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@colesbury |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| self.assertEqual( | ||
| run("reversed", orig["reversed"](list(range(8)))), | ||
| (iter, ([],)) | ||
| (reversed, ([],)) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This test was added from #101769, not sure that it will be okay to change the implementation detail.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think it is OK, see my other comment.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@colesbury Since it touched on the implementation details, I think that we should listen to other core developers' opinions, too.
@gvanrossum Tier 1Tier 2 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if (list == NULL) | ||
| return NULL; | ||
| return Py_BuildValue("N(N)", _PyEval_GetBuiltin(&_Py_ID(iter)), list); | ||
| return Py_BuildValue("N(N)", iter, list); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is the change causing the behavior difference in the test. Previously, an empty iterator would always reduce to the iter builtin, now it will reduce to reversed if it was a reverse iterator. IMO the new behavior is more correct (consistent with the non-empty behavior) and we can consider this a bugfix.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Okay in that case, we need to create backport patches for this.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@gvanrossum @colesbury gentle ping :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think _GUARD_NOT_EXHAUSTED_LIST also needs to be adjusted for the free-threaded build because seq == NULL no longer signifies an exhausted iterator for the free-threaded build. Like with _ITER_JUMP_LIST, I think the deopt can be reduced to:
DEOPT_IF((size_t)it->it_index >= (size_t)PyList_GET_SIZE(seq));
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| #ifdef Py_GIL_DISABLED | ||
| #define LOAD_SSIZE(value) _Py_atomic_load_ssize_relaxed(&value) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The indentation I've seen more commonly in CPython looks like:
#ifdef Py_GIL_DISABLED # define LOAD_SSIZE(value) _Py_atomic_load_ssize_relaxed(&value) ... #endif
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
I trust Sam. My queue is very long and my proessing capacity is reduced. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
e91da7e to
bec617a
Compare
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.