classification
Title: Inheriting from ABCs makes classes slower.
Type: behavior Stage:
Components: Library (Lib) Versions: Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gvanrossum Nosy List: christian.heimes, gvanrossum, jyasskin, nnorwitz, rhettinger
Priority: high Keywords:

Created on 2008-01-08 16:39 by jyasskin, last changed 2008-02-28 04:47 by jyasskin. This issue is now closed.

Files
File name Uploaded Description Edit
trunk_decimal_richcmp.patch christian.heimes, 2008-01-08 18:13
optimize_abc.patch jyasskin, 2008-02-17 02:47 Moves _Abstract into object
Messages (14)
msg59543 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-01-08 16:39
Adding numbers.Real to Decimal's base classes almost doubles the time
its its test suite takes to run. A profile revealed that a large
fraction of that slowdown was in __instancecheck__, but even after
optimizing that, it's still about 25% slower. It looks like the rest of
the slowdown is still in other parts of the isinstance() check. It would
be nice if inheriting from ABCs didn't slow your class down.
msg59544 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-08 16:41
Is this for 2.6 or 3.0?
msg59545 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-01-08 16:43
I've only verified the behavior on 2.6, but I suspect it's true for both.
msg59546 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-08 17:02
__instancecheck__ contains unnecessary code. If we restrict ABCs to new
style classes we could make the function faster:

Old:

    def __instancecheck__(cls, instance):
        """Override for isinstance(instance, cls)."""
        return any(cls.__subclasscheck__(c)
                   for c in {instance.__class__, type(instance)})
New:

    def __instancecheck__(cls, instance):
        """Override for isinstance(instance, cls)."""
        # safe a function call for common case
        if instance.__class__ in cls._abc_cache:
            return True
        return cls.__subclasscheck__(instance.__class__)
msg59548 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-01-08 17:36
What change is responsible for the speedup? The cache check or removing
type(instance) from the set?  Since type(instance) is usually the same
object as instance.__class__, the set will still have only one element
(in particular for Decimal this is the case) so I don't see why that
change is necessary.

It's also important to find out where __instancecheck__ is called; I
don't see where this is happening, so I suspect it's probably somewhere
internal.
msg59550 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-08 18:03
without abc:
$ time ./python Lib/test/regrtest.py test_decimal
real    0m10.113s
user    0m9.685s
sys     0m0.196s

with numbers.Real subclass:
$ time ./python Lib/test/regrtest.py  test_decimal
real    0m16.232s
user    0m15.241s
sys     0m0.384s

Proposed patch:
$ time ./python Lib/test/regrtest.py  test_decimal
real    0m11.128s
user    0m9.533s
sys     0m0.260s

Only with if instance.__class__ in cls._abc_cache: return True
$ time ./python Lib/test/regrtest.py  test_decimal
real    0m11.201s
user    0m10.345s
sys     0m0.292s

Wow, __instancecheck__ must be called a *lot* of times.
msg59552 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-08 18:13
The patch implements the rich cmp slots for <, <=, > and >= required for
numbers.Real.
msg62238 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-02-09 23:02
I measured various implementations of __instancecheck__ using
`./python.exe -m timeit -s 'from rational import Rational; r =
Rational(3, 2)' '...'` on my 2.33 GHz MacBook, with ... replaced by
either isinstance(r, Rational) or isinstance(3, Rational) to measure
both the positive and negative cases. The big win comes from avoiding
the genexp and the set. Then we win smaller amounts by being more
careful about avoiding extra calls to __subclasscheck__ and by inlining
the cache checks.

# Current code
    return any(cls.__subclasscheck__(c)
               for c in set([instance.__class__, type(instance)]))
isinstance(3, Rational): 4.65 usec
isinstance(r, Rational): 7.47 usec

# The best we can do simply in Python
    return cls.__subclasscheck__(instance.__class__)
isinstance(3, Rational): 2.08 usec
isinstance(r, Rational): 1.72 usec

# Preserve behavior, simply
    return (cls.__subclasscheck__(instance.__class__) or
            cls.__subclasscheck__(type(instance)))
isinstance(3, Rational): 3.03 usec
isinstance(r, Rational): 1.8 usec

# Preserve behavior, complexly
    ic = instance.__class__
    if cls.__subclasscheck__(ic):
        return True
    t = type(instance)
    return t is not ic and cls.__subclasscheck__(t)
isinstance(3, Rational): 2.38 usec
isinstance(r, Rational): 1.86 usec

# Inlined for new-style classes
    subclass = instance.__class__
    if subclass in cls._abc_cache:
        return True
    type_ = type(instance)
    if type_ is subclass:
        if (cls._abc_negative_cache_version ==
            ABCMeta._abc_invalidation_counter and
            subclass in cls._abc_negative_cache):
            return False
        return cls.__subclasscheck__(subclass)
    return (cls.__subclasscheck__(subclass) or
            cls.__subclasscheck__(type_))
isinstance(3, Rational): 2.26 usec
isinstance(r, Rational): 1.49 usec
msg62240 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-10 00:18
Whoa!  I thought we had arrived at a decision to leave decimal alone.  
Please do not infect this module with numbers.py.
msg62246 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-02-10 09:26
Right. Decimal was just the place I noticed the problem first. Now it
affects Rational more, but it's really a problem with ABCs in general,
not specific concrete classes.
msg62367 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-02-13 18:00
I've committed the inlined option as r60762.
msg62382 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-02-14 08:05
Guido and I discussed this, and the next step seems to be to rewrite
_Abstract in C and push it into object. For an idea of how much that'll
help, just commenting out _Abstract.__new__ takes the Fraction()
constructor from 9.35us to 6.7us on my box, and the C we need (a new
flag on the class) should run very quickly.
msg62479 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-02-17 02:47
I'd like a second opinion about whether it's a good idea to commit the
attached patch, which moves abc._Abstract into object. Its effect is to
speed

  ./python.exe -m timeit -s 'import abc' -s 'class Foo(object):
__metaclass__=abc.ABCMeta' 'Foo()'

up from 2.5us to 0.201us. For comparison:

  $ ./python.exe -m timeit -s 'import abc' -s 'class Foo(object): pass'
'Foo()'
  10000000 loops, best of 3: 0.203 usec per loop
  $ ./python.exe -m timeit -s 'import abc' -s 'class Foo(object):' -s '
 def __new__(cls): return super(Foo, cls).__new__(cls)' 'Foo()'
  1000000 loops, best of 3: 1.18 usec per loop
  $ ./python.exe -m timeit -s 'import abc' -s 'from decimal import
Decimal' 'Decimal()'
  100000 loops, best of 3: 9.51 usec per loop


After this patch, the only slowdown I can find is an extra .5us in
isinstance, so I think it'll be time to close this bug.
msg63088 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-02-28 04:47
Since there were no comments, I submitted the patch as r61098. I think
we're done here. :)
History
Date User Action Args
2008-02-28 04:47:38jyasskinsetstatus: open -> closed
messages: + msg63088
type: behavior
resolution: fixed
versions: + Python 2.6
2008-02-17 02:47:24jyasskinsetfiles: + optimize_abc.patch
messages: + msg62479
2008-02-14 08:05:15jyasskinsetmessages: + msg62382
2008-02-13 18:00:00jyasskinsetmessages: + msg62367
2008-02-10 09:26:09jyasskinsetmessages: + msg62246
title: Inheriting from ABC slows Decimal down. -> Inheriting from ABCs makes classes slower.
2008-02-10 00:18:22rhettingersetnosy: + rhettinger
messages: + msg62240
2008-02-09 23:02:26jyasskinsetmessages: + msg62238
2008-01-08 18:13:12christian.heimessetfiles: + trunk_decimal_richcmp.patch
messages: + msg59552
2008-01-08 18:03:18christian.heimessetmessages: + msg59550
2008-01-08 17:36:53gvanrossumsetmessages: + msg59548
2008-01-08 17:02:04christian.heimessetassignee: gvanrossum
messages: + msg59546
nosy: + gvanrossum
2008-01-08 16:43:27jyasskinsetmessages: + msg59545
2008-01-08 16:41:33christian.heimessetpriority: high
nosy: + christian.heimes
messages: + msg59544
2008-01-08 16:39:08jyasskincreate