Uh oh!
There was an error while loading. Please reload this page.
GH-132554: "Virtual" iterators - #132555
Conversation
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.
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 |
| static inline _PyStackRef | ||
| PyStackRef_IncrementTaggedIntNoOverflow(_PyStackRef ref) | ||
| { | ||
| assert(ref.bits != (uintptr_t)-1); // Deosn't overflow |
There was a problem hiding this comment.
I don't understand why you use this condition. Should it not be assert(ref.bits + 4 > ref.bits) or something like that?
| return false; | ||
| } | ||
| return PyFunction_Check(PyStackRef_AsPyObjectBorrow(stackref)); | ||
| } |
There was a problem hiding this comment.
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.
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""" | ||
| def foo(x): | ||
| r = 0 |
There was a problem hiding this comment.
I'd add this to make sure the test is testing what the comment is saying.assert(sys.getrefcount(x) == 1)
| _PyStackRef | ||
| PyStackRef_IncrementTaggedIntNoOverflow(_PyStackRef ref) | ||
| { | ||
| assert(ref.index != (uintptr_t)-1); // Overflow |
* 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.
* 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.