[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
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
Merged
+71 −19

Conversation

Copy link
Member

methane commented Jan 13, 2018

Skip raising AttributeError when tp_getattro == PyObject_GenericGetAttr

https://bugs.python.org/issue32544

Copy link
Contributor

ilevkivskyi left a comment

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 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 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 Jan 15, 2018
Author 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 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 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 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.

This comment has been minimized.

This comment has been minimized.

ilevkivskyi Jan 15, 2018
Contributor

@methane Yes this idea looks good.

Copy link
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 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 Jan 15, 2018
Author 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 Jan 15, 2018
Author 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.

methane force-pushed the methane:fast-hasattr branch from c4fdb18 to 477544c Jan 15, 2018
@@ -0,0 +1,3 @@
``hasattr(obj, name)`` and ``getattr(obj, name, default)`` is about 4 times

This comment has been minimized.

ilevkivskyi Jan 16, 2018
Contributor

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

methane merged commit 378edee into python:master Jan 16, 2018
4 checks passed
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 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants