Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
GH-132554: "Virtual" iterators#132555
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
GH-132554: "Virtual" iterators #132555
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markshannon commented Apr 15, 2025 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
417cd59 to 6c955e0Comparemarkshannon commented Apr 16, 2025
Performance is good. Nothing amazing, but a small speedup. Stats show no significant changes |
a4b740d to 025049dCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner commented May 11, 2025
@markshannon I don't think it matters here, but you didn't update in https://github.com/python/cpython/pull/132545/files |
efe465d to f3b9074Comparemarkshannon commented May 22, 2025
Fixed in #134244 |
Include/internal/pycore_stackref.h Outdated
| staticinline_PyStackRef | ||
| PyStackRef_IncrementTaggedIntNoOverflow(_PyStackRefref) | ||
| { | ||
| assert(ref.bits!= (uintptr_t)-1); // Deosn't overflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you use this condition. Should it not be assert(ref.bits + 4 > ref.bits) or something like that?
Include/internal/pycore_stackref.h Outdated
| return false; | ||
| } | ||
| returnPyFunction_Check(PyStackRef_AsPyObjectBorrow(stackref)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it help to define these via a macro? Something like
#define STACKREF_CHECK_FUNC(T) \ static inline bool \ PyStackRef_ ## T ## Check(_PyStackRef stackref) \ if (PyStackRef_IsTaggedInt(stackref)){\ return false; \ } \ return Py ## T ## _Check(PyStackRef_AsPyObjectBorrow(stackref)); \ } ... STACKREF_CHECK_FUNC(Exception); STACKREF_CHECK_FUNC(Code); STACKREF_CHECK_FUNC(Function); There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might not work when want to define variants of Check and CheckExact, though we can always define two macros for that if we go down this route.
| iterable doesn't prematurely free the iterable""" | ||
| deffoo(x): | ||
| r=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add this to make sure the test is testing what the comment is saying.assert(sys.getrefcount(x) == 1)
Python/stackrefs.c Outdated
| _PyStackRef | ||
| PyStackRef_IncrementTaggedIntNoOverflow(_PyStackRefref) | ||
| { | ||
| assert(ref.index!= (uintptr_t)-1); // Overflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do here same as in d9dc597 ?
f6f4e8a into python:mainUh oh!
There was an error while loading. Please reload this page.
* FOR_ITER now pushes either the iterator and NULL or leaves the iterable and pushes tagged zero * NEXT_ITER uses the tagged int as the index into the sequence or, if TOS is NULL, iterates as before.
* FOR_ITER now pushes either the iterator and NULL or leaves the iterable and pushes tagged zero * NEXT_ITER uses the tagged int as the index into the sequence or, if TOS is NULL, iterates as before.
Just a draft PR until I have performance numbers.