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

[RFC] Implement immutability #60

Merged
merged 6 commits into from Aug 20, 2016
Merged

[RFC] Implement immutability #60

merged 6 commits into from Aug 20, 2016

Conversation

@hynek
Copy link
Member

@hynek hynek commented Aug 16, 2016

First shot at immutability to fix #3 and #50.

I would very much like some input from y’all that asked for it: @glyph @radix @Tinche

A couple of notes:

  • I didn’t go for a separate decorator because that would be confusing and harder to implement.
  • I called it frozen because most Pythonista have heard of frozenset. immutable is a bit long, value seems confusingly generic and only understandable to people with FP/theory background.
  • Originally I wanted to just set __setattr__ at the end of __init__ but that doesn’t seem to work unfortunately. So object.setattr it is.

I’m not set on any of those decisions and will happily be converted to any other way. Just please keep in mind, that the feature has to be discoverable and understandable to people who just started programming.

Please paint this bikeshed.

@hynek hynek added this to the 16.1 milestone Aug 16, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Aug 16, 2016

Current coverage is 100% (diff: 100%)

Merging #60 into master will not change coverage

@@           master   #60   diff @@
===================================
  Files           7     8     +1   
  Lines         362   384    +22   
  Methods         0     0          
  Messages        0     0          
  Branches       86    89     +3   
===================================
+ Hits          362   384    +22   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update 5a1814f...c96e6a3

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 16, 2016

Cool :)

Some quick opinions:

  • slots should default to True for frozen classes. The presence of a __dict__ will be useless in normal use anyway. If someone absolutely, positively must change a field (maybe for testing), there's the object.__setattr__ trick.
  • I agree with @glyph in #3 that hashes should be somehow significant. The presence of a hash function is the closest thing Python has to an immutability marker, right? If a class is frozen, I'd consider asserting the presence of __hash__() on all provided arguments in __init__ (maybe just assert it's present and not call it directly to save some time).

You might not like this, but I'd also consider defaulting hash to False for non-frozen classes. Looking at the code base I'm working on at work, I have 44 attrs classes defined. All of them define __hash__() (since I went with the default hash setting), but virtually none of them are actually safe to hash since they either contain dicts or contain classes that contain dicts. And this fact reveals itself at hashing time, not creation time. :) grumbles about the absence of frozendict

I think it'd be valuable for frozen to be more opinionated and guarantee instances can be hashed?

@hynek
Copy link
Member Author

@hynek hynek commented Aug 16, 2016

  1. we cannot change the default of an argument like hash, that would break too much stuff :| I usually just exclude mutable attributes from hash cough.
  2. due to the lack of a frozendict, ISTM that enforcing the hashability of all attributes renders this feature more or less worthless?
  3. And finally a busy @alex wanted to be reminded in 11h. ;)

.. doctest::

>>> @attr.s(frozen=True)

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 16, 2016

Since things are not quite immutable in Python, have you considered frozen='ish'. It's only an extra character ;)

This comment has been minimized.

@Tinche

Tinche Aug 16, 2016
Member

Also remove the comma after "convinced"?

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 16, 2016

Maybe we can have our cake and eat it too? Leave attr.s() as currently implemented and think of it as a low-level, pick-your-exact-settings building block, and introduce @glyph's attr.object() and attr.value() as opinionated, higher level decorators?

attr.value() could just force slots and frozen to True, default hash to True, and insert a check before __init__ to assert hashes on all arguments. Basically we have the building blocks now to do the first post from #3.

@glyph
Copy link
Contributor

@glyph glyph commented Aug 16, 2016

@Tinche I think if we're going to do that, we don't need to discuss it here - this PR stands alone, and would be necessary for that naming strategy anyway :)

from __future__ import absolute_import, division, print_function


class FrozenInstanceError(AttributeError):

This comment has been minimized.

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 16, 2016

@glyph If we're going to do that, I'm 👍 on this.

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 16, 2016

I'm assuming it'd be a compatibility break, and thus a no-no, to make Factory and the validators frozen and slotted?

@hynek
Copy link
Member Author

@hynek hynek commented Aug 17, 2016

Factory was always an opaque object so slotted shouldn’t be a problem. But frozen probably has a runtime impact so unless you can disprove that using benchmarks, I’m not making attrs slower for rather esoteric reasons.

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 17, 2016

Hm, testing on my work Mac, CPython 3.5:

$ pip install -U git+https://github.com/haypo/perf.git#egg=perf
$ # Non-slotted, non-frozen
$ pyperf timeit -g -s "from attr import Factory; l = lambda: 1" "Factory(l)"
....................
402 ns: 10 ############################################
405 ns: 18 ###############################################################################
408 ns:  8 ###################################
410 ns:  9 ########################################
413 ns:  3 #############
416 ns:  4 ##################
418 ns:  2 #########
421 ns:  0 |
424 ns:  1 ####
427 ns:  1 ####
429 ns:  1 ####
432 ns:  0 |
435 ns:  0 |
437 ns:  0 |
440 ns:  0 |
443 ns:  1 ####
445 ns:  0 |
448 ns:  1 ####
451 ns:  0 |
454 ns:  0 |
456 ns:  1 ####

Median +- std dev: 409 ns +- 11 ns
$ Slotted, frozen
$ pyperf timeit -g -s "from attr import Factory; l = lambda: 1" "Factory(l)"
....................
476 ns: 1 ##########
478 ns: 2 ####################
479 ns: 8 ###############################################################################
481 ns: 2 ####################
483 ns: 0 |
484 ns: 8 ###############################################################################
486 ns: 7 #####################################################################
488 ns: 2 ####################
490 ns: 5 #################################################
491 ns: 5 #################################################
493 ns: 5 #################################################
495 ns: 5 #################################################
496 ns: 4 ########################################
498 ns: 1 ##########
500 ns: 2 ####################
501 ns: 0 |
503 ns: 0 |
505 ns: 0 |
507 ns: 1 ##########
508 ns: 1 ##########
510 ns: 1 ##########

Median +- std dev: 489 ns +- 8 ns
$ # Slotted, non-frozen
$ pyperf timeit -g -s "from attr import Factory; l = lambda: 1" "Factory(l)"
....................
294 ns: 5 #################################################
295 ns: 3 ##############################
296 ns: 8 ###############################################################################
297 ns: 7 #####################################################################
298 ns: 5 #################################################
299 ns: 6 ###########################################################
300 ns: 7 #####################################################################
301 ns: 4 ########################################
302 ns: 7 #####################################################################
303 ns: 0 |
304 ns: 1 ##########
305 ns: 2 ####################
306 ns: 0 |
307 ns: 0 |
308 ns: 1 ##########
309 ns: 1 ##########
310 ns: 0 |
311 ns: 1 ##########
312 ns: 1 ##########
313 ns: 0 |
314 ns: 1 ##########

Median +- std dev: 300 ns +- 4 ns

So it would appear there is a slight performance penalty to frozen, but a performance benefit to slots, at least on CPython. We're talking hundreds of nanoseconds, however. Maybe we could optimize the object.__setattr__ part somehow? We're doing an unnecessary lookup (`object.setattr) we could be doing during class init, and inject it into globals. With this change:

# Slotted, frozen with the optimization
$ pyperf timeit -g -s "from attr import Factory; l = lambda: 1" "Factory(l)"
....................
446 ns:  3 ###############
447 ns:  3 ###############
449 ns:  6 ##############################
450 ns: 16 ###############################################################################
451 ns:  3 ###############
452 ns:  3 ###############
454 ns:  6 ##############################
455 ns:  5 #########################
456 ns:  2 ##########
457 ns:  3 ###############
459 ns:  2 ##########
460 ns:  1 #####
461 ns:  2 ##########
462 ns:  0 |
464 ns:  1 #####
465 ns:  1 #####
466 ns:  0 |
467 ns:  0 |
469 ns:  1 #####
470 ns:  1 #####
471 ns:  1 #####

Median +- std dev: 452 ns +- 6 ns

Unfortunately, the perf tool doesn't work on PyPy yet (psf/pyperf#12) so this is all the data I have right now.

@hynek
Copy link
Member Author

@hynek hynek commented Aug 17, 2016

I’ve tried caching too like this:

diff --git a/src/attr/_make.py b/src/attr/_make.py
index b381b09..9a37b10 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -398,10 +398,17 @@ def _add_init(cls, frozen):
     locs = {}
     bytecode = compile(script, unique_filename, "exec")
     attr_dict = dict((a.name, a) for a in attrs)
-    exec_(bytecode, {"NOTHING": NOTHING,
-                     "attr_dict": attr_dict,
-                     "validate": validate,
-                     "_convert": _convert}, locs)
+    globs = {
+        "NOTHING": NOTHING,
+        "attr_dict": attr_dict,
+        "validate": validate,
+        "_convert": _convert
+    }
+    if frozen is True:
+        # Save the lookup overhead in __init__ if we need to circumvent
+        # immutability.
+        globs["_setattr"] = object.__setattr__
+    exec_(bytecode, globs, locs)
     init = locs["__init__"]

     # In order of debuggers like PDB being able to step through the code,
@@ -471,11 +478,11 @@ def _attrs_to_script(attrs, frozen):
     Return a valid Python script of an initializer for *attrs*.

     If *frozen* is True, we cannot set the attributes directly so we use
-    ``object.__setattr__``.
+    a cached ``object.__setattr__``.
     """
     if frozen is True:
         def fmt_setter(attr_name, value):
-            return "object.__setattr__(self, '%(attr_name)s', %(value)s)" % {
+            return "_setattr(self, '%(attr_name)s', %(value)s)" % {
                 "attr_name": attr_name,
                 "value": value,
             }

and I couldn’t find any significant or even consistent performance advantage (MBP late 2013) even with 6 arguments:

➤ pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'])" "C(1,2,3,4,5,6)" # optimized
........................................
716 ns:  1 #####
722 ns:  2 ##########
729 ns:  5 ########################
736 ns: 14 ####################################################################
742 ns: 10 #################################################
749 ns: 11 #####################################################
756 ns:  9 ############################################
763 ns: 13 ###############################################################
769 ns: 11 #####################################################
776 ns: 12 ##########################################################
783 ns:  6 #############################
790 ns:  8 #######################################
796 ns:  6 #############################
803 ns:  4 ###################
810 ns:  1 #####
817 ns:  2 ##########
823 ns:  2 ##########
830 ns:  0 |
837 ns:  1 #####
844 ns:  1 #####
850 ns:  1 #####

Median +- std dev: 766 ns +- 27 ns

➤ pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'])" "C(1,2,3,4,5,6)" # old
........................................
712 ns:  2 #########
720 ns:  1 ####
728 ns: 10 ###########################################
736 ns:  8 ##################################
744 ns: 15 ################################################################
752 ns: 16 ####################################################################
760 ns: 15 ################################################################
768 ns: 13 #######################################################
776 ns: 11 ###############################################
784 ns: 11 ###############################################
792 ns:  9 ######################################
800 ns:  1 ####
808 ns:  0 |
816 ns:  1 ####
824 ns:  3 #############
832 ns:  1 ####
840 ns:  1 ####
848 ns:  0 |
856 ns:  1 ####
864 ns:  0 |
872 ns:  1 ####

Median +- std dev: 764 ns +- 28 ns

Am I doing something wrong?

ISTM that we’re trying to be too clever here and Python does quite a bit of optimizations that we may even be sabotaging.

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 17, 2016

The classes you're testing aren't frozen?

@hynek
Copy link
Member Author

@hynek hynek commented Aug 17, 2016

lol dumbass

OK second try:

➤ pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'], frozen=True)" "C(1,2,3,4,5,6)" # old
........................................
1.61 us:  1 #####
1.63 us:  3 ###############
1.65 us:  7 ####################################
1.67 us:  7 ####################################
1.69 us: 13 ###################################################################
1.71 us:  9 ##############################################
1.72 us: 13 ###################################################################
1.74 us: 10 ####################################################
1.76 us: 11 #########################################################
1.78 us:  6 ###############################
1.80 us:  8 #########################################
1.82 us:  9 ##############################################
1.84 us:  7 ####################################
1.85 us:  6 ###############################
1.87 us:  1 #####
1.89 us:  1 #####
1.91 us:  3 ###############
1.93 us:  2 ##########
1.95 us:  1 #####
1.96 us:  0 |
1.98 us:  2 ##########

Median +- std dev: 1.76 us +- 0.08 us

➤ pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'], frozen=True)" "C(1,2,3,4,5,6)" # new
........................................
1.46 us:  1 ####
1.47 us:  3 ###########
1.48 us:  0 |
1.50 us:  5 ##################
1.51 us:  9 ################################
1.52 us: 10 ###################################
1.53 us: 19 ###################################################################
1.55 us: 10 ###################################
1.56 us: 12 ##########################################
1.57 us:  6 #####################
1.59 us:  7 #########################
1.60 us:  7 #########################
1.61 us: 10 ###################################
1.63 us:  4 ##############
1.64 us:  4 ##############
1.65 us:  5 ##################
1.66 us:  4 ##############
1.68 us:  2 #######
1.69 us:  0 |
1.70 us:  1 ####
1.72 us:  1 ####

Median +- std dev: 1.57 us +- 0.05 us

Holy shit that’s slow in both cases. 😱 More than 2x slower…I know why I’m not gonna use it. :)


As for factories: making them slotted is great and fine with me but frozen? Nah, not worth it.

@hynek hynek force-pushed the immutable branch from de0e20d to 3c646c5 Aug 17, 2016
hynek added a commit that referenced this pull request Aug 17, 2016
Saves memory without downsides.

Ref #60
@hynek
Copy link
Member Author

@hynek hynek commented Aug 17, 2016

I’m kind of still waiting for general approval/feedback btw.

As I’ve made clear, this feature is not for me. I'm not gonna make my code artificially slower just to get sort-of-immutability. I just don’t mutate. ;)


What I’m saying is, that someone has to be happy, so I can’t merge this until things are clear to me, that y’all have what y’all want.

@hynek hynek force-pushed the immutable branch from 3c646c5 to f74744e Aug 17, 2016
@Insoleet
Copy link
Contributor

@Insoleet Insoleet commented Aug 17, 2016

I wonder why immutability is necessary applied to all attributes of a class ? Couldn't immutability only be applied to desired attributes ?

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 17, 2016

@Insoleet it's just a design choice we've agreed on.

A question for someone more knowledgeable than I (i.e. other people in the conversation): is there a dirtier, faster hack we could be using instead?

@hynek hynek force-pushed the immutable branch from f74744e to c96e6a3 Aug 19, 2016
@hynek
Copy link
Member Author

@hynek hynek commented Aug 19, 2016

Circumventing the descriptor gave us some more vroom vroom:

➤ pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', ['x', 'y', 'z'], frozen=True)" "C(1,2,3)" # old
........................................
763 ns: 28 ####################################################################
767 ns: 16 #######################################
772 ns:  5 ############
777 ns:  5 ############
781 ns:  6 ###############
786 ns:  5 ############
791 ns:  6 ###############
795 ns: 11 ###########################
800 ns: 11 ###########################
804 ns:  3 #######
809 ns:  4 ##########
814 ns:  2 #####
818 ns:  4 ##########
823 ns:  4 ##########
828 ns:  4 ##########
832 ns:  1 ##
837 ns:  2 #####
842 ns:  0 |
846 ns:  0 |
851 ns:  1 ##
856 ns:  2 #####

Median +- std dev: 785 ns +- 24 ns

vs.

➤ pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', ['x', 'y', 'z'], frozen=True)" "C(1,2,3)" # more new
........................................
684 ns:  1 #####
688 ns:  7 ################################
693 ns:  5 #######################
697 ns:  1 #####
701 ns:  7 ################################
705 ns: 13 ###########################################################
709 ns: 15 ####################################################################
713 ns:  9 #########################################
717 ns: 11 ##################################################
721 ns: 11 ##################################################
725 ns: 10 #############################################
729 ns:  7 ################################
733 ns: 10 #############################################
737 ns:  5 #######################
741 ns:  3 ##############
745 ns:  2 #########
749 ns:  0 |
753 ns:  1 #####
757 ns:  1 #####
761 ns:  0 |
765 ns:  1 #####

Median +- std dev: 718 ns +- 16 ns

But I’m pretty sure we’re arriving at a point of diminishing returns.

@hynek
Copy link
Member Author

@hynek hynek commented Aug 20, 2016

OK everyone, last call for conceptual feedback/vetoes?

if frozen is True:
lines.append(
"_setattr = _cached_setattr.__get__(self, self.__class__)"

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 20, 2016

Would it be useful to add a comment explaining what exactly this does?

This comment has been minimized.

@hynek

hynek Aug 20, 2016
Author Member

Is

            # Circumvent the __setattr__ descriptor to save one lookup per
            # assignment.

accurate @Tinche?

This comment has been minimized.

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 20, 2016

Personally I think this is useful and should go in. Maybe I won't use it for classes that get instantiated a lot in inner loops, but for long-lived classes where you'd like to avoid deepcopying frozen is surely the way to go. (Think of attr attributes, where the deepcopy totally dominates the performance of fields().)

Maybe add a note to the docs that a frozen __init__ will be slightly slower than a non-frozen one? Also, a slotted __init__ turns out to be faster than non-slotted. On CPython 3.5.

@Tinche
Copy link
Member

@Tinche Tinche commented Aug 20, 2016

Unfortunately, as stated, perf doesn't work on PyPy yet, so I tried using normal timeit. Comparing CPython 3.5 and PyPy 5.3.1:

> .venv/bin/python -m timeit -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'], frozen=True)" "C(1,2,3,4,5,6)"
1000000 loops, best of 3: 1.55 usec per loop
> .venv/bin/python -m timeit -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'])" "C(1,2,3,4,5,6)"
1000000 loops, best of 3: 0.935 usec per loop
> 
> .venv-pypy/bin/python -m timeit -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'], frozen=True)" "C(1,2,3,4,5,6)"
100000000 loops, best of 3: 0.012 usec per loop
> .venv-pypy/bin/python -m timeit -s "import attr; C = attr.make_class('C', ['x', 'y', 'z', 'a', 'b', 'c'])" "C(1,2,3,4,5,6)"
1000000000 loops, best of 3: 0.00092 usec per loop

There's an overhead on PyPy too. 12 ns vs 0.9 ns? Not even sure these numbers are to be trusted. In any case I could spare 12 ns to instantiate a class.

@hynek hynek merged commit cfa6d2e into master Aug 20, 2016
3 checks passed
3 checks passed
codecov/project 100% (+0.00%) compared to 5a1814f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@hynek hynek deleted the immutable branch Aug 20, 2016
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.

6 participants
You can’t perform that action at this time.