From de3b2d4001fe7cff5ccf8f892e6b4747c79250b7 Mon Sep 17 00:00:00 2001 From: Saul Shanabrook Date: Thu, 23 May 2024 23:31:29 -0400 Subject: [PATCH 1/4] Add initial implementation of removing _CHECK_FUNCTION w/ out all tests --- Include/internal/pycore_optimizer.h | 3 +++ Lib/test/test_capi/test_opt.py | 18 ++++++++++++++++++ Objects/funcobject.c | 6 +++--- Python/optimizer_analysis.c | 16 ++++++++++++++++ Python/optimizer_bytecodes.c | 17 +++++++++++++++++ Python/optimizer_cases.c.h | 14 ++++++++++++++ Python/optimizer_symbols.c | 22 +++++++++++++++++++++- 7 files changed, 92 insertions(+), 4 deletions(-) diff --git a/Include/internal/pycore_optimizer.h b/Include/internal/pycore_optimizer.h index 76123987ac99f56..6154705bb04a6d4 100644 --- a/Include/internal/pycore_optimizer.h +++ b/Include/internal/pycore_optimizer.h @@ -33,6 +33,7 @@ struct _Py_UopsSymbol { int flags; // 0 bits: Top; 2 or more bits: Bottom PyTypeObject *typ; // Borrowed reference PyObject *const_val; // Owned reference (!) + uint32_t function_version; }; #define UOP_FORMAT_TARGET 0 @@ -123,9 +124,11 @@ extern _Py_UopsSymbol *_Py_uop_sym_new_const(_Py_UOpsContext *ctx, PyObject *con extern _Py_UopsSymbol *_Py_uop_sym_new_null(_Py_UOpsContext *ctx); extern bool _Py_uop_sym_has_type(_Py_UopsSymbol *sym); extern bool _Py_uop_sym_matches_type(_Py_UopsSymbol *sym, PyTypeObject *typ); +extern bool _Py_uop_sym_matches_function_version(_Py_UopsSymbol *sym, uint32_t version); extern void _Py_uop_sym_set_null(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym); extern void _Py_uop_sym_set_non_null(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym); extern void _Py_uop_sym_set_type(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, PyTypeObject *typ); +extern void _Py_uop_sym_set_function_version(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, uint32_t version); extern void _Py_uop_sym_set_const(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, PyObject *const_val); extern bool _Py_uop_sym_is_bottom(_Py_UopsSymbol *sym); extern int _Py_uop_sym_truthiness(_Py_UopsSymbol *sym); diff --git a/Lib/test/test_capi/test_opt.py b/Lib/test/test_capi/test_opt.py index 0491ff9b84d486c..c5d12ac6f369d9c 100644 --- a/Lib/test/test_capi/test_opt.py +++ b/Lib/test/test_capi/test_opt.py @@ -1334,5 +1334,23 @@ def test_modified_local_is_seen_by_optimized_code(self): self.assertEqual(s, 1024.0) + def test_guard_type_version_removed(self): + def thing(f): + x = 0 + for _ in range(100): + x += f() + f() + return x + + # Specify an unused arg in the function so that + # CALL_PY_GENERAL is used over CALL_PY_EXACT_ARGS + res, ex = self._run_with_optimizer(thing, lambda _=1: 1) + opnames = list(iter_opnames(ex)) + self.assertIsNotNone(ex) + self.assertEqual(res, 200) + + guard_type_version_count = opnames.count("_CHECK_FUNCTION_VERSION") + self.assertEqual(guard_type_version_count, 1) + if __name__ == "__main__": unittest.main() + diff --git a/Objects/funcobject.c b/Objects/funcobject.c index 4e78252052932c9..52045f6184947b2 100644 --- a/Objects/funcobject.c +++ b/Objects/funcobject.c @@ -70,7 +70,7 @@ PyFunction_AddWatcher(PyFunction_WatchCallback callback) { PyInterpreterState *interp = _PyInterpreterState_GET(); assert(interp->_initialized); - for (int i = 0; i < FUNC_MAX_WATCHERS; i++) { + for (int i = 1; i < FUNC_MAX_WATCHERS; i++) { if (interp->func_watchers[i] == NULL) { interp->func_watchers[i] = callback; interp->active_func_watchers |= (1 << i); @@ -346,11 +346,11 @@ _PyFunction_LookupByVersion(uint32_t version, PyObject **p_code) if (slot->code) { assert(PyCode_Check(slot->code)); PyCodeObject *code = (PyCodeObject *)slot->code; - if (code->co_version == version) { + if (code->co_version == version && p_code) { *p_code = slot->code; } } - else { + else if (p_code) { *p_code = NULL; } if (slot->func && slot->func->func_version == version) { diff --git a/Python/optimizer_analysis.c b/Python/optimizer_analysis.c index e5d3793bd4d2045..eb3af5afd452454 100644 --- a/Python/optimizer_analysis.c +++ b/Python/optimizer_analysis.c @@ -79,7 +79,17 @@ increment_mutations(PyObject* dict) { * so we don't need to check that they haven't been used */ #define BUILTINS_WATCHER_ID 0 #define GLOBALS_WATCHER_ID 1 +#define FUNCTION_WATCHER_ID 0 +static int +function_watcher_callback( + PyFunction_WatchEvent event, + PyFunctionObject *func, + PyObject *new_value) +{ + _Py_Executors_InvalidateDependency(_PyInterpreterState_GET(), func, 1); + return 0; +} static int globals_watcher_callback(PyDict_WatchEvent event, PyObject* dict, PyObject* key, PyObject* new_value) @@ -167,6 +177,10 @@ remove_globals(_PyInterpreterFrame *frame, _PyUOpInstruction *buffer, if (interp->dict_state.watchers[GLOBALS_WATCHER_ID] == NULL) { interp->dict_state.watchers[GLOBALS_WATCHER_ID] = globals_watcher_callback; } + if (interp->func_watchers[GLOBALS_WATCHER_ID] == NULL) { + interp->active_func_watchers |= 0x1; + interp->func_watchers[FUNCTION_WATCHER_ID] = function_watcher_callback; + } for (int pc = 0; pc < buffer_size; pc++) { _PyUOpInstruction *inst = &buffer[pc]; int opcode = inst->opcode; @@ -310,9 +324,11 @@ remove_globals(_PyInterpreterFrame *frame, _PyUOpInstruction *buffer, #define sym_has_type _Py_uop_sym_has_type #define sym_get_type _Py_uop_sym_get_type #define sym_matches_type _Py_uop_sym_matches_type +#define sym_matches_function_version _Py_uop_sym_matches_function_version #define sym_set_null(SYM) _Py_uop_sym_set_null(ctx, SYM) #define sym_set_non_null(SYM) _Py_uop_sym_set_non_null(ctx, SYM) #define sym_set_type(SYM, TYPE) _Py_uop_sym_set_type(ctx, SYM, TYPE) +#define sym_set_function_version(SYM, VERSION) _Py_uop_sym_set_function_version(ctx, SYM, VERSION) #define sym_set_const(SYM, CNST) _Py_uop_sym_set_const(ctx, SYM, CNST) #define sym_is_bottom _Py_uop_sym_is_bottom #define sym_truthiness _Py_uop_sym_truthiness diff --git a/Python/optimizer_bytecodes.c b/Python/optimizer_bytecodes.c index e5c982befb2411a..bff8fed85931a2a 100644 --- a/Python/optimizer_bytecodes.c +++ b/Python/optimizer_bytecodes.c @@ -21,11 +21,13 @@ typedef struct _Py_UOpsAbstractFrame _Py_UOpsAbstractFrame; #define sym_new_const _Py_uop_sym_new_const #define sym_new_null _Py_uop_sym_new_null #define sym_matches_type _Py_uop_sym_matches_type +#define sym_matches_function_version _Py_uop_sym_matches_function_version #define sym_get_type _Py_uop_sym_get_type #define sym_has_type _Py_uop_sym_has_type #define sym_set_null(SYM) _Py_uop_sym_set_null(ctx, SYM) #define sym_set_non_null(SYM) _Py_uop_sym_set_non_null(ctx, SYM) #define sym_set_type(SYM, TYPE) _Py_uop_sym_set_type(ctx, SYM, TYPE) +#define sym_set_function_version(SYM, VERSION) _Py_uop_sym_set_function_version(ctx, SYM, VERSION) #define sym_set_const(SYM, CNST) _Py_uop_sym_set_const(ctx, SYM, CNST) #define sym_is_bottom _Py_uop_sym_is_bottom #define frame_new _Py_uop_frame_new @@ -95,6 +97,21 @@ dummy_func(void) { res = sym_new_null(ctx); } + op(_CHECK_FUNCTION_VERSION, (func_version/2, callable, unused, unused[oparg] -- callable, unused, unused[oparg])) { + if (sym_matches_function_version(callable, func_version)) { + REPLACE_OP(this_instr, _NOP, 0, 0); + } else { + PyFunctionObject *function = _PyFunction_LookupByVersion(func_version, NULL); + // if the type is null, it was not found in the cache (there was a conflict) + // with the key, in which case we can't trust the version + if (function) { + sym_set_function_version(callable, func_version); + _Py_BloomFilter_Add(dependencies, function); + } + + } + } + op(_GUARD_BOTH_INT, (left, right -- left, right)) { if (sym_matches_type(left, &PyLong_Type)) { if (sym_matches_type(right, &PyLong_Type)) { diff --git a/Python/optimizer_cases.c.h b/Python/optimizer_cases.c.h index 30ed011a83eb6e4..17e9ac84298e83d 100644 --- a/Python/optimizer_cases.c.h +++ b/Python/optimizer_cases.c.h @@ -1471,6 +1471,20 @@ } case _CHECK_FUNCTION_VERSION: { + _Py_UopsSymbol *callable; + callable = stack_pointer[-2 - oparg]; + uint32_t func_version = (uint32_t)this_instr->operand; + if (sym_matches_function_version(callable, func_version)) { + REPLACE_OP(this_instr, _NOP, 0, 0); + } else { + PyFunctionObject *function = _PyFunction_LookupByVersion(func_version, NULL); + // if the type is null, it was not found in the cache (there was a conflict) + // with the key, in which case we can't trust the version + if (function) { + sym_set_function_version(callable, func_version); + _Py_BloomFilter_Add(dependencies, function); + } + } break; } diff --git a/Python/optimizer_symbols.c b/Python/optimizer_symbols.c index e546eef306eeca0..be3c7293b99ca9a 100644 --- a/Python/optimizer_symbols.c +++ b/Python/optimizer_symbols.c @@ -52,7 +52,8 @@ static inline int get_lltrace(void) { static _Py_UopsSymbol NO_SPACE_SYMBOL = { .flags = IS_NULL | NOT_NULL | NO_SPACE, .typ = NULL, - .const_val = NULL + .const_val = NULL, + .function_version = 0, }; _Py_UopsSymbol * @@ -76,6 +77,7 @@ sym_new(_Py_UOpsContext *ctx) self->flags = 0; self->typ = NULL; self->const_val = NULL; + self->function_version = 0; return self; } @@ -152,6 +154,24 @@ _Py_uop_sym_set_type(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, PyTypeObject *ty } } +void +_Py_uop_sym_set_function_version(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, uint32_t version) +{ + sym->function_version = version; +} + +uint32_t +_Py_uop_sym_get_function_version(_Py_UopsSymbol *sym) +{ + return sym->function_version; +} + +bool +_Py_uop_sym_matches_function_version(_Py_UopsSymbol *sym, uint32_t version) +{ + return _Py_uop_sym_get_function_version(sym) == version; +} + void _Py_uop_sym_set_const(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, PyObject *const_val) { From 5934f50dc01cc032213b2e674d85d21aaf726f30 Mon Sep 17 00:00:00 2001 From: Saul Shanabrook Date: Wed, 29 May 2024 20:35:54 -0400 Subject: [PATCH 2/4] Update tests to use function exact --- Lib/test/test_capi/test_opt.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_capi/test_opt.py b/Lib/test/test_capi/test_opt.py index c5d12ac6f369d9c..66d19a18159214e 100644 --- a/Lib/test/test_capi/test_opt.py +++ b/Lib/test/test_capi/test_opt.py @@ -1333,23 +1333,34 @@ def test_modified_local_is_seen_by_optimized_code(self): self.assertIs(type(s), float) self.assertEqual(s, 1024.0) - - def test_guard_type_version_removed(self): + def test_guard_function_version_removed(self): def thing(f): x = 0 for _ in range(100): x += f() + f() return x - # Specify an unused arg in the function so that - # CALL_PY_GENERAL is used over CALL_PY_EXACT_ARGS - res, ex = self._run_with_optimizer(thing, lambda _=1: 1) + res, ex = self._run_with_optimizer(thing, lambda: 1) opnames = list(iter_opnames(ex)) self.assertIsNotNone(ex) self.assertEqual(res, 200) + self.assertEqual(opnames.count("_CHECK_FUNCTION_VERSION"), 1) + + def test_guard_function_version_invalidated(self): + def thing(f): + x = 0 + for _ in range(100): + x += f() + x += f() + return x + + def fn(): + return 1 + res, ex = self._run_with_optimizer(thing, fn) + self.assertTrue(ex.is_valid()) + fn.__code__ = fn.__code__ + self.assertFalse(ex.is_valid()) - guard_type_version_count = opnames.count("_CHECK_FUNCTION_VERSION") - self.assertEqual(guard_type_version_count, 1) if __name__ == "__main__": unittest.main() From d901565c095fb1d0f2464932fd01e8bb8665ebd6 Mon Sep 17 00:00:00 2001 From: Saul Shanabrook Date: Wed, 29 May 2024 20:37:19 -0400 Subject: [PATCH 3/4] Remove extra newline --- Lib/test/test_capi/test_opt.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_capi/test_opt.py b/Lib/test/test_capi/test_opt.py index 66d19a18159214e..8ecf1e3c17856c6 100644 --- a/Lib/test/test_capi/test_opt.py +++ b/Lib/test/test_capi/test_opt.py @@ -1364,4 +1364,3 @@ def fn(): if __name__ == "__main__": unittest.main() - From d9717dd9d352f7ddcb4cd74ea76a5b9ab6f1e7ee Mon Sep 17 00:00:00 2001 From: Saul Shanabrook Date: Mon, 10 Jun 2024 11:51:02 -0400 Subject: [PATCH 4/4] Don't invalidate on function destruction --- Lib/test/test_capi/test_opt.py | 3 ++- Python/optimizer_analysis.c | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_capi/test_opt.py b/Lib/test/test_capi/test_opt.py index 73fb18493d435c4..1f47e8b622b8dac 100644 --- a/Lib/test/test_capi/test_opt.py +++ b/Lib/test/test_capi/test_opt.py @@ -1480,6 +1480,7 @@ def fn(a): fn(A()) + def test_guard_function_version_removed(self): def thing(f): x = 0 @@ -1505,7 +1506,7 @@ def fn(): return 1 res, ex = self._run_with_optimizer(thing, fn) self.assertTrue(ex.is_valid()) - fn.__code__ = fn.__code__ + fn.__code__ = fn.__code__.replace(co_name="new_name") self.assertFalse(ex.is_valid()) diff --git a/Python/optimizer_analysis.c b/Python/optimizer_analysis.c index 600e5260ddbf44b..de762c188bac0e5 100644 --- a/Python/optimizer_analysis.c +++ b/Python/optimizer_analysis.c @@ -88,6 +88,10 @@ function_watcher_callback( PyFunctionObject *func, PyObject *new_value) { + // Don't invalidate if we are destroying funciton + if (event == PyFunction_EVENT_DESTROY) { + return 0; + } _Py_Executors_InvalidateDependency(_PyInterpreterState_GET(), func, 1); return 0; }