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

[WIP] Frozen attribute support #172

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Tinche
Copy link
Member

commented Mar 28, 2017

WIP. Do not merge yet :)

Got initial frozen attribute support working for dict classes using the magic of multiple inheritance.

Now to think of something clever for slot classes.

Tinche added some commits Apr 1, 2017

@Tinche

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2017

Alright, we support slot classes too now. The information below is valid for slot classes.

There is zero overhead for __init__.

pyperf timeit --rigorous -g --duplicate 10 -s "import attr; B = attr.make_class('B', {'a': attr.ib(frozen=True), 'b': attr.ib(frozen=True)}, slots=True)" 'B(1, 2)'

Before: Mean +- std dev: 410 ns +- 11 ns
After: Mean +- std dev: 409 ns +- 11 ns

Unfortunately, attribute access is much slower:

pyperf timeit --rigorous -g --duplicate 10 -s "import attr; B = attr.make_class('B', {'a': attr.ib(frozen=True), 'b': attr.ib(frozen=True)}, slots=True); b = B(1, 2)" 'b.b'

Before: Mean +- std dev: 35.8 ns +- 0.8 ns
After: Mean +- std dev: 391 ns +- 17 ns

We wrap the original slot descriptor to block mutation, but we pass through access. However, this involves a pure Python method invocation. I don't know how to avoid this :(

@hynek

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

Nice, factor 10. :| I don’t think adding RCEs (i.e. write some C) would really help here in any way? Very generally speaking, I could live with an optional C to speed things up I guess. I’m just not eager for it.

@Tinche

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

It would probably help. We would need a customized version of this: https://github.com/python/cpython/blob/fff9a31a91283c39c363af219e595eab7d4da6f7/Objects/descrobject.c#L594

Can Cython help us out here somehow?

@hynek

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

I'd hope so. I'm not adding actual C.

@Tinche

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

Huh, playing around with Cython a little, managed to get the overhead to just ~3x. 25.8 ns to 74.7 ns.

The numbers are a little different since I'm on my work computer atm.

@hynek

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

Interesting.

Generally I want to give everyone maximum flexibility so the Cython code would go into a separate package (attrs-accel?) and everything has to be implemented also in pure Python. I bet it’s faster on PyPy anyways. :)

@Tinche

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

I've left a post on the cython-users mailing list asking for a little advice.

In the mean time, fiddling around, I believe I've managed to get a Cython solution with only 5% overhead (27 ns vs 26 ns).

@Tinche

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

Here's the link to the post since it finally got approved, just to be thorough. https://groups.google.com/forum/#!topic/cython-users/QaQDNLE-76Y

@Tinche

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2017

Ok, confirming that the speed penalty for frozen slot attributes with the Cython accelerator is within 10%.

Non-frozen access: 35.3 ns +- 1.1 ns
Cython frozen access: 38.5 ns +- 1.2 ns
Python frozen access: 368 ns +- 15 ns

I think this makes the idea of having some Cython code in attrs worthwhile, and when we have this infrastructure in place we might want to offer accelerated versions of other components too. I agree all Cython code should be an optimized reimplementation of Python code - native code should be strictly optional.

How do we go about this though? We can have a separate package for Cythonized components, but having a single sdist that cleanly installs even without a C compiler (and just falls back to Python versions) would be less maintenance - is it possible? We will also need to get Travis to test both versions of the code, and make wheels where we can.

Somewhat unrelated: it occured to me that for frozen dict classes, we can generate a slightly different __init__:

def __init__(self, a, b):
    __dict__ = self.__dict__
    __dict__['a'] = a
    __dict__['b'] = b

This is functionally identical to the way we do it now, with the cached object.setattr, right? This approach totally avoids the __init__ speed penalty for frozen dict classes.

@hynek

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

This is pretty amazing. That’s gonna be the flagship feature of attrs 17.2 I reckon.

Maybe we should brainstorm some what makes sense to accelerate etc.

@smarie

This comment has been minimized.

Copy link

commented Apr 24, 2017

@Tinche , just out of curiosity what would be the overhead of transforming all attributes that require either frozen access or have convertors and/or validators, into python @property ? It is fairly easy to add a property to a class :

setattr(clazz, attr_name, property(fget=getter_fun, fset=setter_fun))

With getter_fun being for example a function or lambda with direct access to the field

getter_fun = lambda self: getattr(self, private_attr_name)

And setter_fun being a function or lambda including frozen access check or conv + validators as usual for non-frozen attrs - the same you have to day in the __init__ .

Properties seem to now exist for both python 2 and 3.

I don't know how to make that consistent with the base Attribute class, maybe Attribute could directly inherit from property? Or a flavour of attribute (PropertyAttribute) could inherit.
Let me know if that is just stupid, I'm relatively new to Python..

(relates to requirement 160)

@Tinche

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

It's not stupid, the problem is the getter is slow - it involves a pure Python method call.

The current approach is similar to @property, but lower level. @property under the hood creates a descriptor which invokes the getter and returns the value; we create a descriptor that wraps the existing descriptor on a very low level and delegates getting to it.

@smarie

This comment has been minimized.

Copy link

commented Apr 24, 2017

ok, thanks for the clarification !

@hynek hynek changed the title Frozen attribute support [WIP] Frozen attribute support Aug 4, 2017

@Infernion

This comment has been minimized.

Copy link

commented May 2, 2018

Is there any progress on this feature?

@RonnyPfannschmidt RonnyPfannschmidt referenced this pull request May 7, 2018

Open

Naive implementation for frozen class hooks #379

2 of 6 tasks complete
@ittVannak

This comment has been minimized.

Copy link

commented Nov 27, 2018

Bump? Enjoyed reading the history of this request - is this still in the plan for attrs?

@wsanchez

This comment has been minimized.

Copy link

commented Nov 27, 2018

Folks: if there is progress to report, you'll see it here. If we decide not to do it, the ticket will be closed, hopefully with an explanation. Otherwise you can assume it's still being considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.