-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-134584: Eliminate redundant refcounting from _BINARY_OP_ADD_UNICODE #135817
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.
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
Changes from 1 commit
d5afe15
46b423f
c901079
ea112a5
d95aee9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |||
| import unittest | ||||
| import gc | ||||
| import os | ||||
| import random | ||||
| import string | ||||
|
|
||||
| import _opcode | ||||
|
|
||||
|
|
@@ -2362,6 +2364,23 @@ def testfunc(n): | |||
| self.assertNotIn("_GUARD_TOS_INT", uops) | ||||
| self.assertNotIn("_GUARD_NOS_INT", uops) | ||||
|
|
||||
| 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 | ||||
|
|
||||
| testfunc(TIER2_THRESHOLD) | ||||
|
|
||||
| ex = get_first_executor(testfunc) | ||||
| self.assertIsNotNone(ex) | ||||
| uops = get_opnames(ex) | ||||
|
|
||||
| self.assertIn("_POP_TOP_NOP", uops) | ||||
|
|
||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I thought of a better test: you can use this one but change for str, and assert that the cpython/Lib/test/test_capi/test_opt.py Line 2310 in 46b423f
|
||||
| def test_attr_promotion_failure(self): | ||||
| # We're not testing for any specific uops here, just | ||||
| # testing it doesn't crash. | ||||
|
|
||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ typedef struct _Py_UOpsAbstractFrame _Py_UOpsAbstractFrame; | |
| #define sym_new_tuple _Py_uop_sym_new_tuple | ||
| #define sym_tuple_getitem _Py_uop_sym_tuple_getitem | ||
| #define sym_tuple_length _Py_uop_sym_tuple_length | ||
| #define sym_is_immortal _Py_uop_sym_is_immortal | ||
| #define sym_is_immortal _Py_uop_symbol_is_immortal | ||
| #define sym_new_compact_int _Py_uop_sym_new_compact_int | ||
| #define sym_is_compact_int _Py_uop_sym_is_compact_int | ||
| #define sym_new_truthiness _Py_uop_sym_new_truthiness | ||
|
|
@@ -318,7 +318,7 @@ dummy_func(void) { | |
| } | ||
| } | ||
|
|
||
| op(_BINARY_OP_ADD_UNICODE, (left, right -- res)) { | ||
| op(_BINARY_OP_ADD_UNICODE, (left, right -- res, l, r)) { | ||
| if (sym_is_const(ctx, left) && sym_is_const(ctx, right)) { | ||
| assert(PyUnicode_CheckExact(sym_get_const(ctx, left))); | ||
| assert(PyUnicode_CheckExact(sym_get_const(ctx, right))); | ||
|
|
@@ -327,10 +327,14 @@ dummy_func(void) { | |
| goto error; | ||
| } | ||
| res = sym_new_const(ctx, temp); | ||
| l = left; | ||
| r = right; | ||
tomasr8 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Py_DECREF(temp); | ||
| } | ||
| else { | ||
| res = sym_new_type(ctx, &PyUnicode_Type); | ||
| l = left; | ||
| r = right; | ||
| } | ||
|
||
| } | ||
|
|
||
|
|
@@ -561,11 +565,39 @@ dummy_func(void) { | |
| value = PyJitRef_Borrow(sym_new_const(ctx, ptr)); | ||
| } | ||
|
|
||
| op(_POP_TOP_UNICODE, (value -- )) { | ||
| if (PyJitRef_IsBorrowed(value) || | ||
| sym_is_immortal(PyJitRef_Unwrap(value))) { | ||
| REPLACE_OP(this_instr, _POP_TOP_NOP, 0, 0); | ||
| } | ||
| } | ||
|
|
||
| op(_COPY, (bottom, unused[oparg-1] -- bottom, unused[oparg-1], top)) { | ||
| assert(oparg > 0); | ||
| top = bottom; | ||
| } | ||
|
|
||
| op(_POP_TOP, (value -- )) { | ||
| PyTypeObject *typ = sym_get_type(value); | ||
| PyObject *const_val = sym_get_const(ctx, value); | ||
| if (PyJitRef_IsBorrowed(value) || | ||
| (const_val != NULL && _Py_IsImmortal(const_val))) { | ||
| REPLACE_OP(this_instr, _POP_TOP_NOP, 0, 0); | ||
| } | ||
| else if (typ == &PyLong_Type) { | ||
| REPLACE_OP(this_instr, _POP_TOP_INT, 0, 0); | ||
| } | ||
| else if (typ == &PyFloat_Type) { | ||
| REPLACE_OP(this_instr, _POP_TOP_FLOAT, 0, 0); | ||
| } | ||
| else if (typ == &PyUnicode_Type) { | ||
| REPLACE_OP(this_instr, _POP_TOP_UNICODE, 0, 0); | ||
| } | ||
| else if (typ == &PyBool_Type) { | ||
| REPLACE_OP(this_instr, _POP_TOP_NOP, 0, 0); | ||
| } | ||
| } | ||
|
|
||
| op(_SWAP, (bottom, unused[oparg-2], top -- bottom, unused[oparg-2], top)) { | ||
| JitOptRef temp = bottom; | ||
| bottom = top; | ||
|
|
@@ -803,7 +835,7 @@ dummy_func(void) { | |
| } | ||
|
|
||
| op(_RETURN_VALUE, (retval -- res)) { | ||
| JitOptRef temp = retval; | ||
| JitOptRef temp = PyJitRef_Wrap(PyJitRef_Unwrap(retval)); | ||
| DEAD(retval); | ||
| SAVE_STACK(); | ||
| ctx->frame->stack_pointer = stack_pointer; | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also add
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this code, It will be replaced to
_POP_TOP_NOPbecause_POP_TOP_UNICODEwill be optimized to_POP_TOP_NOP.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, do you think it makes sense to test
_POP_TOP_UNICODEseparately? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a test for that in my original PR already.