GH-99298: Don't perform jumps before error handling #99299
Conversation
brandtbucher
added
type-bug
| @@ -1152,6 +1152,8 @@ dummy_func( | |||
| PyObject *name = GETITEM(names, oparg); | |||
| next_instr--; | |||
| if (_Py_Specialize_StoreAttr(owner, next_instr, name)) { | |||
| // "undo" the rewind so end up in the correct handler: | |||
| next_instr++; | |||
There was a problem hiding this comment.
Adding any code to a conditional block with just a goto adds extra branches.
It's not so important for these slower instructions, but its best avoided in general.
What we generally do is if (cond) goto fixup_error; where fixup_error does the necessary fix before jumping to error:. The C compiler will probably do this for us, but I think it best to be explicit.
There was a problem hiding this comment.
Yeah, see my comment at the top of this PR. _Py_Specialize_StoreAttr and _Py_Specialize_LoadAttr don't really need error handling at all, so the next thing I'm going to do it make them return void like the others in 3.12. Then we don't need the branch at all. :)
I'm hesitant to add additional labels and gotos to the 3.11 backport, especially since this is pretty cold code. Let me know if you think I should, though.
There was a problem hiding this comment.
Ok, leave this for this PR.
Maybe make a PR for 3.12 that removes the branch entirely?
My next steps:
_Py_Specialize_LoadAttrand_Py_Specialize_StoreAttrdon't actually need to handle errors (we can just fail to specialize "unready" types, which are probably super rare in practice).