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

bpo-32544: Speed up hasattr() and getattr() #5173

Merged
merged 7 commits into from Jan 16, 2018

Conversation

Projects
None yet
5 participants
@methane
Member

methane commented Jan 13, 2018

Skip raising AttributeError when tp_getattro == PyObject_GenericGetAttr

https://bugs.python.org/issue32544

@ilevkivskyi

I like the idea!

(Sorry I am a bit slow with ABCMeta, because I moved to another country.)

@@ -536,6 +536,8 @@ PyAPI_FUNC(int) PyObject_SetAttr(PyObject *, PyObject *, PyObject *);
PyAPI_FUNC(int) PyObject_HasAttr(PyObject *, PyObject *);
#ifndef Py_LIMITED_API
PyAPI_FUNC(int) _PyObject_IsAbstract(PyObject *);
/* Same to PyObject_GetAttr(), but don't raise AttributeError. */

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 13, 2018

Contributor

I think this should read "Same as ..."

@@ -567,7 +569,7 @@ PyAPI_FUNC(int) PyObject_CallFinalizerFromDealloc(PyObject *);
/* Same as PyObject_Generic{Get,Set}Attr, but passing the attributes
dict as the last parameter. */
PyAPI_FUNC(PyObject *)
_PyObject_GenericGetAttrWithDict(PyObject *, PyObject *, PyObject *);
_PyObject_GenericGetAttrWithDict(PyObject *, PyObject *, PyObject *, int);

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 13, 2018

Contributor

Would this change be backward incompatible? Or is this function is not covered by the backwards compatibility guarantees?

This comment has been minimized.

@methane

methane Jan 15, 2018

Member

AFAIK, APIs starting with underscore are private.
Third party libs shouldn't expect backward compatibility.
I don't know should I add Include/internal/object.h or not.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 15, 2018

Contributor

OK, I see. But I don't know about Include/internal/object.h either. Probably @serhiy-storchaka can advise on this.

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

Changing the signature of _PyObject_GenericGetAttrWithDict() LGTM. As for Include/internal/object.h, left this to other issue.

ret = (*tp->tp_getattro)(v, name);
}
}
else if (tp->tp_getattr != NULL) {

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 13, 2018

Contributor

It looks like there is some code duplication with PyObject_GetAttr, does it make sense to factor it out?

This comment has been minimized.

@methane

This comment has been minimized.

@serhiy-storchaka

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 15, 2018

Contributor

@methane Yes this idea looks good.

@ilevkivskyi

This comment has been minimized.

Contributor

ilevkivskyi commented Jan 13, 2018

Also I think we need a NEWS item for this.

}
if (tp->tp_getattro != NULL) {
if (tp->tp_getattro == PyObject_GenericGetAttr) {
ret = _PyObject_GenericGetAttrWithDict(v, name, NULL, 1);

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

Can _PyObject_GenericGetAttrWithDict() raise an AttributeError? If can't, it is better to just return the result here. PyErr_ExceptionMatches(PyExc_AttributeError) has a non-zero cost, in some cases it can be significant. For example see bpo-31336.

This comment has been minimized.

@methane

methane Jan 15, 2018

Member

Yes, it may call descriptor. Any exception can be raised.
suppress=1 only suppress AttributeError raised from _PyObject_GenericGetAttrWithDict
directly.

This comment has been minimized.

@methane

methane Jan 15, 2018

Member

I changed _PyObject_GenericGetAttrWithDict() to suppress AttributeError
from descriptors too.
Since _PyObject_GenericGetAttrWithDict is performance critical function,
I'll run benchmark suite before merge this.

@@ -0,0 +1,3 @@
``hasattr(obj, name)`` and ``getattr(obj, name, default)`` is about 4 times

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 16, 2018

Contributor

I am not a native speaker, but I would use "are" here instead of "is".

@methane methane merged commit 378edee into python:master Jan 16, 2018

4 checks passed

bedevere/issue-number Issue number 32544 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@methane methane deleted the methane:fast-hasattr branch Jan 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment