bpo-45107: Make LOAD_METHOD_CLASS safer and faster, clean up comments (GH-28177)

* Improve comments

* Check cls is a type, remove dict calculation
This commit is contained in:
Ken Jin 2021-09-17 18:47:36 +08:00 committed by GitHub
parent b0a6ede3d0
commit 70bed6f993
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 9 additions and 15 deletions

View file

@ -4496,6 +4496,7 @@ check_eval_breaker:
} }
TARGET(LOAD_METHOD_MODULE): { TARGET(LOAD_METHOD_MODULE): {
/* LOAD_METHOD, for module methods */
assert(cframe.use_tracing == 0); assert(cframe.use_tracing == 0);
PyObject *owner = TOP(); PyObject *owner = TOP();
PyObject *res; PyObject *res;
@ -4515,15 +4516,9 @@ check_eval_breaker:
_PyObjectCache *cache2 = &caches[-2].obj; _PyObjectCache *cache2 = &caches[-2].obj;
PyObject *cls = TOP(); PyObject *cls = TOP();
PyTypeObject *cls_type = Py_TYPE(cls); DEOPT_IF(!PyType_Check(cls), LOAD_METHOD);
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, DEOPT_IF(((PyTypeObject *)cls)->tp_version_tag != cache1->tp_version,
LOAD_METHOD); LOAD_METHOD);
assert(cache1->dk_version_or_hint != 0);
assert(cache1->tp_version != 0); assert(cache1->tp_version != 0);
STAT_INC(LOAD_METHOD, hit); STAT_INC(LOAD_METHOD, hit);

View file

@ -974,20 +974,19 @@ _Py_Specialize_LoadMethod(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name,
// Fall through. // Fall through.
} // Else owner is maybe a builtin with no dict, or __slots__. Doesn't matter. } // 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 /* `descr` is borrowed. This is safe for methods (even inherited ones from
* it's deleted. This is safe for methods (even inherited ones from super * super classes!) as long as tp_version_tag is validated for two main reasons:
* 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 * 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. * 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. * `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 * 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. * of caching a borrowed descriptor. The MCACHE infrastructure does all the
* E.g. it invalidates on any MRO modification, on any type object * heavy lifting for us. E.g. it invalidates tp_version_tag on any MRO
* change along said MRO, etc. (see PyType_Modified usages in typeobject.c). * modification, on any type object change along said MRO, etc. (see
* The type method cache has been working since Python 2.6 and it's * PyType_Modified usages in typeobject.c). The MCACHE has been
* battle-tested. * working since Python 2.6 and it's battle-tested.
*/ */
cache2->obj = descr; cache2->obj = descr;
cache1->dk_version_or_hint = keys_version; cache1->dk_version_or_hint = keys_version;