Skip to content
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

Speedup _setattr usage and fix slight performance regressions #991

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/991.change.rst
@@ -0,0 +1 @@
Fix slight performance regression in classes with custom ``__setattr__`` and speedup even more.
4 changes: 2 additions & 2 deletions docs/how-does-it-work.rst
Expand Up @@ -94,9 +94,9 @@ This is (still) slower than a plain assignment:
-s "import attr; C = attr.make_class('C', ['x', 'y', 'z'], slots=True, frozen=True)" \
"C(1, 2, 3)"
.........................................
Mean +- std dev: 450 ns +- 26 ns
Mean +- std dev: 425 ns +- 16 ns

So on a laptop computer the difference is about 230 nanoseconds (1 second is 1,000,000,000 nanoseconds).
So on a laptop computer the difference is about 200 nanoseconds (1 second is 1,000,000,000 nanoseconds).
It's certainly something you'll feel in a hot loop but shouldn't matter in normal code.
Pick what's more important to you.

Expand Down
24 changes: 17 additions & 7 deletions src/attr/_make.py
Expand Up @@ -915,7 +915,7 @@ def slots_setstate(self, state):
"""
Automatically created by attrs.
"""
__bound_setattr = _obj_setattr.__get__(self, Attribute)
__bound_setattr = _obj_setattr.__get__(self)
for name, value in zip(state_attr_names, state):
__bound_setattr(name, value)

Expand Down Expand Up @@ -2007,6 +2007,7 @@ def _make_init(
cache_hash,
base_attr_map,
is_exc,
needs_cached_setattr,
has_cls_on_setattr,
attrs_init,
)
Expand All @@ -2019,7 +2020,7 @@ def _make_init(
if needs_cached_setattr:
# Save the lookup overhead in __init__ if we need to circumvent
# setattr hooks.
globs["_setattr"] = _obj_setattr
globs["_cached_setattr_get"] = _obj_setattr.__get__

init = _make_method(
"__attrs_init__" if attrs_init else "__init__",
Expand All @@ -2036,15 +2037,15 @@ def _setattr(attr_name, value_var, has_on_setattr):
"""
Use the cached object.setattr to set *attr_name* to *value_var*.
"""
return "_setattr(self, '%s', %s)" % (attr_name, value_var)
return "_setattr('%s', %s)" % (attr_name, value_var)


def _setattr_with_converter(attr_name, value_var, has_on_setattr):
"""
Use the cached object.setattr to set *attr_name* to *value_var*, but run
its converter first.
"""
return "_setattr(self, '%s', %s(%s))" % (
return "_setattr('%s', %s(%s))" % (
attr_name,
_init_converter_pat % (attr_name,),
value_var,
Expand Down Expand Up @@ -2086,6 +2087,7 @@ def _attrs_to_init_script(
cache_hash,
base_attr_map,
is_exc,
needs_cached_setattr,
has_cls_on_setattr,
attrs_init,
):
Expand All @@ -2101,6 +2103,14 @@ def _attrs_to_init_script(
if pre_init:
lines.append("self.__attrs_pre_init__()")

if needs_cached_setattr:
lines.append(
# Circumvent the __setattr__ descriptor to save one lookup per
# assignment.
# Note _setattr will be used again below if cache_hash is True
"_setattr = _cached_setattr_get(self)"
)

if frozen is True:
if slots is True:
fmt_setter = _setattr
Expand Down Expand Up @@ -2315,7 +2325,7 @@ def fmt_setter_with_converter(
if frozen:
if slots:
# if frozen and slots, then _setattr defined above
init_hash_cache = "_setattr(self, '%s', %s)"
init_hash_cache = "_setattr('%s', %s)"
else:
# if frozen and not slots, then _inst_dict defined above
init_hash_cache = "_inst_dict['%s'] = %s"
Expand Down Expand Up @@ -2428,7 +2438,7 @@ def __init__(
)

# Cache this descriptor here to speed things up later.
bound_setattr = _obj_setattr.__get__(self, Attribute)
bound_setattr = _obj_setattr.__get__(self)

# Despite the big red warning, people *do* instantiate `Attribute`
# themselves.
Expand Down Expand Up @@ -2525,7 +2535,7 @@ def __setstate__(self, state):
self._setattrs(zip(self.__slots__, state))

def _setattrs(self, name_values_pairs):
bound_setattr = _obj_setattr.__get__(self, Attribute)
bound_setattr = _obj_setattr.__get__(self)
for name, value in name_values_pairs:
if name != "metadata":
bound_setattr(name, value)
Expand Down
5 changes: 3 additions & 2 deletions tests/test_functional.py
Expand Up @@ -745,6 +745,7 @@ class D(C):

src = inspect.getsource(D.__init__)

assert "_setattr(self, 'x', x)" in src
assert "_setattr(self, 'y', y)" in src
assert "_setattr = _cached_setattr_get(self)" in src
assert "_setattr('x', x)" in src
assert "_setattr('y', y)" in src
assert object.__setattr__ != D.__setattr__