From c2a36c464b271205876db29e1148106e1a99cc03 Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Mon, 6 Sep 2021 00:24:59 +0800 Subject: [PATCH 1/5] Remove dict calculation and check --- Python/ceval.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index bf95d50b629582..cc7961bac41dea 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4517,13 +4517,8 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr PyObject *cls = TOP(); PyTypeObject *cls_type = Py_TYPE(cls); assert(cls_type->tp_dictoffset > 0); - PyObject *dict = *(PyObject **) ((char *)cls + cls_type->tp_dictoffset); - // Don't care if no dict -- tp_version_tag should catch anything wrong. - DEOPT_IF(dict != NULL && ((PyDictObject *)dict)->ma_keys->dk_version != - cache1->dk_version_or_hint, LOAD_METHOD); DEOPT_IF(((PyTypeObject *)cls)->tp_version_tag != cache1->tp_version, LOAD_METHOD); - assert(cache1->dk_version_or_hint != 0); assert(cache1->tp_version != 0); STAT_INC(LOAD_METHOD, hit); From af0dd27cc8af7b446b11d6aa43cff0266a011209 Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Mon, 6 Sep 2021 00:33:54 +0800 Subject: [PATCH 2/5] Improve comments --- Python/ceval.c | 1 + Python/specialize.c | 15 +++++++-------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index cc7961bac41dea..7c80c25c8dec43 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4496,6 +4496,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr } TARGET(LOAD_METHOD_MODULE): { + /* LOAD_METHOD, for module methods */ assert(cframe.use_tracing == 0); PyObject *owner = TOP(); PyObject *res; diff --git a/Python/specialize.c b/Python/specialize.c index 6c76fa6b05fbbe..e067101f5804cd 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -976,20 +976,19 @@ _Py_Specialize_LoadMethod(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name, // Fall through. } // Else owner is maybe a builtin with no dict, or __slots__. Doesn't matter. - /* `descr` is borrowed. Just check tp_version_tag before accessing in case - * it's deleted. This is safe for methods (even inherited ones from super - * classes!) as long as tp_version_tag is validated for two main reasons: + /* `descr` is borrowed. This is safe for methods (even inherited ones from + * super classes!) as long as tp_version_tag is validated for two main reasons: * * 1. The class will always hold a reference to the method so it will * usually not be GC-ed. Should it be deleted in Python, e.g. * `del obj.meth`, tp_version_tag will be invalidated, because of reason 2. * * 2. The pre-existing type method cache (MCACHE) uses the same principles - * of caching a borrowed descriptor. It does all the heavy lifting for us. - * E.g. it invalidates on any MRO modification, on any type object - * change along said MRO, etc. (see PyType_Modified usages in typeobject.c). - * The type method cache has been working since Python 2.6 and it's - * battle-tested. + * of caching a borrowed descriptor. The MCACHE infrastructure does all the + * heavy lifting for us. E.g. it invalidates tp_version_tag on any MRO + * modification, on any type object change along said MRO, etc. (see + * PyType_Modified usages in typeobject.c). The MCACHE has been + * working since Python 2.6 and it's battle-tested. */ cache2->obj = descr; cache1->dk_version_or_hint = keys_version; From 9b83ccbb915a51f89a18773f78b5e51fce27bd8f Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Wed, 15 Sep 2021 23:17:46 +0800 Subject: [PATCH 3/5] make checks stricter --- Python/ceval.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Python/ceval.c b/Python/ceval.c index 7c80c25c8dec43..577dfaa3cb70cf 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4518,8 +4518,15 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr PyObject *cls = TOP(); PyTypeObject *cls_type = Py_TYPE(cls); assert(cls_type->tp_dictoffset > 0); + PyObject *dict = *(PyObject **) ((char *)cls + cls_type->tp_dictoffset); + // All types should have a dict. dk_version is also an identity check + // to ensure `cls` is really a type object. Otherwise, reading + // tp_version_tag may segfault. + DEOPT_IF(dict == NULL || ((PyDictObject *)dict)->ma_keys->dk_version != + cache1->dk_version_or_hint, LOAD_METHOD); DEOPT_IF(((PyTypeObject *)cls)->tp_version_tag != cache1->tp_version, LOAD_METHOD); + assert(cache1->dk_version_or_hint != 0); assert(cache1->tp_version != 0); STAT_INC(LOAD_METHOD, hit); From 82814e94eae6b6c1ff897394bf289ae5132d940c Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Thu, 16 Sep 2021 21:50:52 +0800 Subject: [PATCH 4/5] Check cls is a type, remove dict calculation --- Python/ceval.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index 577dfaa3cb70cf..334cf2feb9509b 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4516,14 +4516,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr _PyObjectCache *cache2 = &caches[-2].obj; PyObject *cls = TOP(); - PyTypeObject *cls_type = Py_TYPE(cls); - assert(cls_type->tp_dictoffset > 0); - PyObject *dict = *(PyObject **) ((char *)cls + cls_type->tp_dictoffset); - // All types should have a dict. dk_version is also an identity check - // to ensure `cls` is really a type object. Otherwise, reading - // tp_version_tag may segfault. - DEOPT_IF(dict == NULL || ((PyDictObject *)dict)->ma_keys->dk_version != - cache1->dk_version_or_hint, LOAD_METHOD); + DEOPT_IF(!PyType_Check(cls), LOAD_METHOD); DEOPT_IF(((PyTypeObject *)cls)->tp_version_tag != cache1->tp_version, LOAD_METHOD); assert(cache1->dk_version_or_hint != 0); From 6ae6effc2930a55cdae7e89daa9e7f8211e517c1 Mon Sep 17 00:00:00 2001 From: Fidget-Spinner <28750310+Fidget-Spinner@users.noreply.github.com> Date: Thu, 16 Sep 2021 21:54:41 +0800 Subject: [PATCH 5/5] remove useless assert --- Python/ceval.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/ceval.c b/Python/ceval.c index 334cf2feb9509b..ab692fd8ded157 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4519,7 +4519,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr DEOPT_IF(!PyType_Check(cls), LOAD_METHOD); DEOPT_IF(((PyTypeObject *)cls)->tp_version_tag != cache1->tp_version, LOAD_METHOD); - assert(cache1->dk_version_or_hint != 0); assert(cache1->tp_version != 0); STAT_INC(LOAD_METHOD, hit);