Uh oh!
There was an error while loading. Please reload this page.
gh-139757: Add BINARY_OP_SUBSCR_USTR_INT - #143389
Conversation
passes the tests and brings bm_tomli back to normal, i.e. 15 % speedup
Fidget-Spinner
commented
Jan 3, 2026
I verified a 9% speedup on this PR for the Apparently, we regressed earlier in https://github.com/python/cpython/pull/140800/files. This caused the 10% slowdown in tomli loads. Which seemingly uses a lot of unicode strings. I'm going to merge this PR to remove the perf regression. |
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Just need tests and two comments. Thanks!
| res = PyStackRef_FromPyObjectBorrow(res_o); | ||
| } | ||
| macro(BINARY_OP_SUBSCR_NCSTR_INT) = |
There was a problem hiding this comment.
When you have the time, please change then name to BINARY_OP_SUBSCR_USTR_INT.
| } | ||
| macro(BINARY_OP_SUBSCR_NCSTR_INT) = | ||
| _GUARD_TOS_INT + _GUARD_NOS_UNICODE + unused/5 + _BINARY_OP_SUBSCR_NCSTR_INT + _POP_TOP_INT + POP_TOP; |
There was a problem hiding this comment.
Is POP_TOP_UNICODE instead of POP_TOP safe here?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertNotIn("_GUARD_TOS_UNICODE", uops) | ||
| self.assertIn("_BINARY_OP_ADD_UNICODE", uops) | ||
| def test_binary_subcsr_ustr_int_narrows_to_str(self): |
There was a problem hiding this comment.
I thought of paramterizing test_binary_subcsr_str_int_narrows_to_str and test_binary_op_subscr_str_int to get rid of duplication, but it resulted in worse readable code, because the input and the expectations have to be varied ...
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner
commented
Jan 4, 2026
Sorry for leaving 3 separate review comments instead of one review. I only picked up on some of these while looking over them again. |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot
commented
Jan 4, 2026
|
Since #140800
BINARY_OP_SUBSCR_STR_INTonly specializes for compact ASCII strings. Let's introduceBINARY_OP_SUBSCR_USTR_INTto specialize again for reading an ASCII character from any string.