Skip to content
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.

Sign up for GitHub

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-91432: Remove the iterator_exhausted_no_error label #96517

Merged
merged 2 commits into from
Sep 7, 2022
+20 −16
Merged

GH-91432: Remove the iterator_exhausted_no_error label #96517

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83e7223
Don't goto iterator_exhausted_no_error
brandtbucher Sep 2, 2022
49deea4
Feedback from PR review
brandtbucher Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Don't goto iterator_exhausted_no_error
@brandtbucher
brandtbucher committed Sep 2, 2022
commit 83e72230a765d25707c547cc84b540ee22c9be21
31 changes: 16 additions & 15 deletions Python/ceval.c
View file
Original file line number Diff line number Diff line change
@@ -3810,9 +3810,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
}
_PyErr_Clear(tstate);
}
iterator_exhausted_no_error:
/* iterator ended normally */
assert(!_PyErr_Occurred(tstate));
Py_DECREF(POP());
JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER + oparg);
DISPATCH();
@@ -3839,19 +3837,20 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
DEOPT_IF(Py_TYPE(it) != &PyListIter_Type, FOR_ITER);
STAT_INC(FOR_ITER, hit);
PyListObject *seq = it->it_seq;
if (seq == NULL) {
goto iterator_exhausted_no_error;
}
if (it->it_index < PyList_GET_SIZE(seq)) {
PyObject *next = PyList_GET_ITEM(seq, it->it_index++);
Py_INCREF(next);
PUSH(next);
JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER);
NOTRACE_DISPATCH();
if (seq) {
if (it->it_index < PyList_GET_SIZE(seq)) {
PyObject *next = PyList_GET_ITEM(seq, it->it_index++);
Py_INCREF(next);
PUSH(next);
JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER);
NOTRACE_DISPATCH();
}
it->it_seq = NULL;
Py_DECREF(seq);
}
it->it_seq = NULL;
Py_DECREF(seq);
goto iterator_exhausted_no_error;
Py_DECREF(POP());
JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER + oparg);
DISPATCH();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this can be NOTRACE_DISPATCH() now, right? Likewise in FOR_ITER_RANGE

Copy link
Member Author

@brandtbucher brandtbucher Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@brandtbucher brandtbucher Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#96636

}

TARGET(FOR_ITER_RANGE) {
@@ -3862,7 +3861,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
_Py_CODEUNIT next = next_instr[INLINE_CACHE_ENTRIES_FOR_ITER];
assert(_PyOpcode_Deopt[_Py_OPCODE(next)] == STORE_FAST);
if (r->index >= r->len) {
goto iterator_exhausted_no_error;
Py_DECREF(POP());
JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER + oparg);
DISPATCH();
}
long value = (long)(r->start +
(unsigned long)(r->index++) * r->step);