GH-132554: "Virtual" iterators - #132555
Conversation
Sorry, something went wrong.
|
Performance is good. Nothing amazing, but a small speedup. Stats show no significant changes |
Sorry, something went wrong.
|
@markshannon I don't think it matters here, but you didn't update in https://github.com/python/cpython/pull/132545/files Py_TAG_BITS to 3. It's still 1. I know it "technically" doesn't matter because we use a bitshift to zero it out, but we should still update it to be consistent. |
Sorry, something went wrong.
Fixed in #134244 |
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| 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);
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
| _PyStackRef | ||
| PyStackRef_IncrementTaggedIntNoOverflow(_PyStackRef ref) | ||
| { | ||
| assert(ref.index != (uintptr_t)-1); // Overflow |
There was a problem hiding this comment.
Do here same as in d9dc597 ?
Sorry, something went wrong.
* 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.