Repository navigation
GH-91432: Remove the iterator_exhausted_no_error label - #96517
Conversation
brandtbucher
added
skip news
interpreter-core
| goto iterator_exhausted_no_error; | ||
| Py_DECREF(POP()); | ||
| JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER + oparg); | ||
| DISPATCH(); |
There was a problem hiding this comment.
I believe this can be NOTRACE_DISPATCH() now, right? Likewise in FOR_ITER_RANGE
There was a problem hiding this comment.
I honestly have a hunch that a bunch of the existing NOTRACE_DISPATCH() usage is incorrect and could trigger assertion failures (but haven't had time to look into it yet). I don't think NOTRACE_DISPATCH() is safe if we decref an untrusted value (like a list containing arbitrary elements) since the finalizer could start tracing. Right?
So in the range case it's safe, but probably not the list case (if my hunch is correct). What do you think?
There was a problem hiding this comment.
I'm curious how much perf win we're getting from the NOTRACE_DISPATCH() stuff. It seems really tricky to get right, and it only saves one arithmetic operation on oparg.
There was a problem hiding this comment.
I thought we were assuming that __del__ methods execute at an unspecified time, so if they turn on tracing, it shouldn't matter whether we actually start tracing a few instructions later.
I guess the unfortunate scenario would be where a __del__ method turns on tracing, then sees a few instructions traced, then returns to the Py_DECREF call site, and then can take a couple of instructions before it starts tracing that caller. I'm not sure whether that is acceptable.
There was a problem hiding this comment.
Hm, well in that case we should at least remove all those assertions at the start of the quickened instructions. I also want to try benchmarking a branch were we just use DISPATCH() for everything, and see how much slower that is. If it's not much slower, it may just make sense to keep the tracing logic consistent for all instructions.
There was a problem hiding this comment.
Want to make a new issue to review our use of NOTRACE_DISPATCH()?
I found a ~1% slowdown replacing all uses of it with DISPATCH().
If PEP 669 is accepted it will no longer be needed, as DISPATCH() will become the same as NOTRACE_DISPATCH()
4 participants
There's not a very good reason why this tiny region of code needs to be shared between three different instructions. I find it impairs readability, and creates a dependency between the logic of the three opcodes that doesn't really need to be there.