gh-134584: Eliminate redundant refcounting from _BINARY_OP_ADD_UNICODE #135817
Conversation
|
Maybe I have to wait #135761 first |
Python/optimizer_bytecodes.c
Outdated
| l = left; | ||
| r = right; | ||
| Py_DECREF(temp); | ||
| } | ||
| else { | ||
| res = sym_new_type(ctx, &PyUnicode_Type); | ||
| l = left; | ||
| r = right; | ||
| } |
There was a problem hiding this comment.
This doesn't need to be in the branch. You can write this at the end of the instruction here.
That way, we only need to have one l = left; r = right
| self.assertIsNotNone(ex) | ||
| uops = get_opnames(ex) | ||
|
|
||
| self.assertIn("_POP_TOP_NOP", uops) |
There was a problem hiding this comment.
Maybe also add
self.assertIn("_BINARY_OP_ADD_UNICODE", uops)to make sure this is the op that's actually generated?
Should we also test for _POP_TOP_UNICODE?
There was a problem hiding this comment.
In this code, It will be replaced to _POP_TOP_NOP because _POP_TOP_UNICODE will be optimized to _POP_TOP_NOP .
There was a problem hiding this comment.
Yup, do you think it makes sense to test _POP_TOP_UNICODE separately? That is, the case when it is not optimized to _POP_TOP_NOP? (not sure how difficult it'll be to come up with a test case though)
There was a problem hiding this comment.
I have a test for that in my original PR already.
Lib/test/test_capi/test_opt.py
Outdated
| def test_store_fast_pop_top_specialize_unicode(self): | ||
| def random_str(n): | ||
| return ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(n)) | ||
| def testfunc(n): | ||
| y = random_str(32) | ||
| for _ in range(n): | ||
| x = y + y # _POP_TOP | ||
| x = None # _POP_TOP_NOP 8000 | ||
|
|
||
| testfunc(TIER2_THRESHOLD) | ||
|
|
||
| ex = get_first_executor(testfunc) | ||
| self.assertIsNotNone(ex) | ||
| uops = get_opnames(ex) | ||
|
|
||
| self.assertIn("_POP_TOP_NOP", uops) | ||
|
|
There was a problem hiding this comment.
Sorry I thought of a better test: you can use this one but change for str, and assert that the _POP_TOP_UNICODE uop is inside
cpython/Lib/test/test_capi/test_opt.py
Line 2310 in 46b423f
|
Sorry I just merged my PR in #135761. You'll have to rebase/merge in changes! |
|
@Fidget-Spinner Done! |
There was a problem hiding this comment.
This adds an extra field to every frame, which is likely to cost more than the improvement this PR would otherwise bring.
If we apply this change to every BINARY_OP, then it might be worth it.
Having said that, TOS caching might solve that problem.
If we can guarantee that each uop that would overflow the stack always leaves the necessary number of outputs in registers and that we never spill those values to memory,
than we can do this without the extra stack space.
Let's wait until TOS is in, and then we can investigate if that approach makes sense.
|
When you're done making the requested changes, leave the comment: |
Uh oh!
There was an error while loading. Please reload this page.