-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
gh-115999: Add free-threaded specialization for TO_BOOL
#126616
Changes from all commits
62b0307
e32f2f0
6798366
e45778b
bafabd7
aa8a81c
1f4a350
23e190e
30020b9
b1f1c71
a213390
e68dc65
b27f916
249efd1
c08f8b7
6bc4c90
542e4dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
|
@@ -2665,101 +2665,106 @@ _Py_Specialize_Send(_PyStackRef receiver_st, _Py_CODEUNIT *instr) | |
cache->counter = adaptive_counter_cooldown(); | ||
} | ||
|
||
#ifdef Py_STATS | ||
static int | ||
to_bool_fail_kind(PyObject *value) | ||
{ | ||
if (PyByteArray_CheckExact(value)) { | ||
return SPEC_FAIL_TO_BOOL_BYTEARRAY; | ||
} | ||
if (PyBytes_CheckExact(value)) { | ||
return SPEC_FAIL_TO_BOOL_BYTES; | ||
} | ||
if (PyDict_CheckExact(value)) { | ||
return SPEC_FAIL_TO_BOOL_DICT; | ||
} | ||
if (PyFloat_CheckExact(value)) { | ||
return SPEC_FAIL_TO_BOOL_FLOAT; | ||
} | ||
if (PyMemoryView_Check(value)) { | ||
return SPEC_FAIL_TO_BOOL_MEMORY_VIEW; | ||
} | ||
if (PyAnySet_CheckExact(value)) { | ||
return SPEC_FAIL_TO_BOOL_SET; | ||
} | ||
if (PyTuple_CheckExact(value)) { | ||
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. Maybe not for this PR to address, but why is the tuple type handled here? I see no specialization for tuple (or did I miss something?). Tuple is immutable, so it should be relatively easy to add 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. IIRC, we added specializaiton based on metric that is measured at https://github.com/faster-cpython/benchmarking-public. 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. I agree that tuple case is easy to add but that would be based on how many boolean operations are existed for the tuple at the real world workload. |
||
return SPEC_FAIL_TO_BOOL_TUPLE; | ||
} | ||
return SPEC_FAIL_OTHER; | ||
} | ||
#endif // Py_STATS | ||
|
||
static int | ||
check_type_always_true(PyTypeObject *ty) | ||
{ | ||
PyNumberMethods *nb = ty->tp_as_number; | ||
if (nb && nb->nb_bool) { | ||
return SPEC_FAIL_TO_BOOL_NUMBER; | ||
} | ||
PyMappingMethods *mp = ty->tp_as_mapping; | ||
if (mp && mp->mp_length) { | ||
return SPEC_FAIL_TO_BOOL_MAPPING; | ||
} | ||
PySequenceMethods *sq = ty->tp_as_sequence; | ||
if (sq && sq->sq_length) { | ||
return SPEC_FAIL_TO_BOOL_SEQUENCE; | ||
} | ||
return 0; | ||
} | ||
|
||
void | ||
_Py_Specialize_ToBool(_PyStackRef value_o, _Py_CODEUNIT *instr) | ||
{ | ||
assert(ENABLE_SPECIALIZATION); | ||
assert(ENABLE_SPECIALIZATION_FT); | ||
assert(_PyOpcode_Caches[TO_BOOL] == INLINE_CACHE_ENTRIES_TO_BOOL); | ||
_PyToBoolCache *cache = (_PyToBoolCache *)(instr + 1); | ||
PyObject *value = PyStackRef_AsPyObjectBorrow(value_o); | ||
uint8_t specialized_op; | ||
if (PyBool_Check(value)) { | ||
instr->op.code = TO_BOOL_BOOL; | ||
specialized_op = TO_BOOL_BOOL; | ||
goto success; | ||
} | ||
if (PyLong_CheckExact(value)) { | ||
instr->op.code = TO_BOOL_INT; | ||
specialized_op = TO_BOOL_INT; | ||
goto success; | ||
} | ||
if (PyList_CheckExact(value)) { | ||
instr->op.code = TO_BOOL_LIST; | ||
specialized_op = TO_BOOL_LIST; | ||
goto success; | ||
} | ||
if (Py_IsNone(value)) { | ||
instr->op.code = TO_BOOL_NONE; | ||
specialized_op = TO_BOOL_NONE; | ||
goto success; | ||
} | ||
if (PyUnicode_CheckExact(value)) { | ||
instr->op.code = TO_BOOL_STR; | ||
specialized_op = TO_BOOL_STR; | ||
goto success; | ||
} | ||
if (PyType_HasFeature(Py_TYPE(value), Py_TPFLAGS_HEAPTYPE)) { | ||
PyNumberMethods *nb = Py_TYPE(value)->tp_as_number; | ||
if (nb && nb->nb_bool) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_NUMBER); | ||
goto failure; | ||
} | ||
PyMappingMethods *mp = Py_TYPE(value)->tp_as_mapping; | ||
if (mp && mp->mp_length) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_MAPPING); | ||
goto failure; | ||
} | ||
PySequenceMethods *sq = Py_TYPE(value)->tp_as_sequence; | ||
if (sq && sq->sq_length) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_SEQUENCE); | ||
goto failure; | ||
} | ||
if (!PyUnstable_Type_AssignVersionTag(Py_TYPE(value))) { | ||
unsigned int version = 0; | ||
int err = _PyType_Validate(Py_TYPE(value), check_type_always_true, &version); | ||
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. This seems to be adding a fair bit of overhead and indirection. 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.
This adds one extra function call (the call to
This might be possible, but would require using something like a sequence lock to do correctly, which is more complicated and harder to reason about than a solution that acquires the lock around the validation that needs to be performed. |
||
if (err < 0) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_OUT_OF_VERSIONS); | ||
goto failure; | ||
} | ||
uint32_t version = type_get_version(Py_TYPE(value), TO_BOOL); | ||
if (version == 0) { | ||
else if (err > 0) { | ||
SPECIALIZATION_FAIL(TO_BOOL, err); | ||
goto failure; | ||
} | ||
instr->op.code = TO_BOOL_ALWAYS_TRUE; | ||
write_u32(cache->version, version); | ||
|
||
assert(err == 0); | ||
assert(version); | ||
write_u32(cache->version, version); | ||
specialized_op = TO_BOOL_ALWAYS_TRUE; | ||
goto success; | ||
} | ||
#ifdef Py_STATS | ||
if (PyByteArray_CheckExact(value)) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_BYTEARRAY); | ||
goto failure; | ||
} | ||
if (PyBytes_CheckExact(value)) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_BYTES); | ||
goto failure; | ||
} | ||
if (PyDict_CheckExact(value)) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_DICT); | ||
goto failure; | ||
} | ||
if (PyFloat_CheckExact(value)) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_FLOAT); | ||
goto failure; | ||
} | ||
if (PyMemoryView_Check(value)) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_MEMORY_VIEW); | ||
goto failure; | ||
} | ||
if (PyAnySet_CheckExact(value)) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_SET); | ||
goto failure; | ||
} | ||
if (PyTuple_CheckExact(value)) { | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_TO_BOOL_TUPLE); | ||
goto failure; | ||
} | ||
SPECIALIZATION_FAIL(TO_BOOL, SPEC_FAIL_OTHER); | ||
#endif // Py_STATS | ||
|
||
SPECIALIZATION_FAIL(TO_BOOL, to_bool_fail_kind(value)); | ||
failure: | ||
STAT_INC(TO_BOOL, failure); | ||
instr->op.code = TO_BOOL; | ||
cache->counter = adaptive_counter_backoff(cache->counter); | ||
unspecialize(instr); | ||
return; | ||
success: | ||
STAT_INC(TO_BOOL, success); | ||
cache->counter = adaptive_counter_cooldown(); | ||
specialize(instr, specialized_op); | ||
} | ||
|
||
#ifdef Py_STATS | ||
|
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.
This should be surrounded by
#ifdef Py_STATS
. We don't want to be calling this unless stats are enabled.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.
@mpage
I have same concerns with this change, this change was first introduced at #126414, do we need to make it as dummy function for the default build?
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 don't think the performance overhead of calling this will matter, but if it does, we could replace it with a dummy function that returns a special sentinel value indicating that stats are disabled.
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.
Both clang (18.18) and GCC (11.5.0) eliminate the call to
to_bool_fail_kind
if stats are disabled because the result of the call is unused.