Inheriting from ABCs makes classes slower.

[jyasskin]

BPO	1762
Nosy	@gvanrossum, @rhettinger, @tiran
Files	trunk_decimal_richcmp.patch optimize_abc.patch: Moves _Abstract into object

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gvanrossum'
closed_at = <Date 2008-02-28.04:47:38.883>
created_at = <Date 2008-01-08.16:39:08.868>
labels = ['type-bug', 'library']
title = 'Inheriting from ABCs makes classes slower.'
updated_at = <Date 2008-02-28.04:47:38.852>
user = 'https://bugs.python.org/jyasskin'

bugs.python.org fields:

activity = <Date 2008-02-28.04:47:38.852>
actor = 'jyasskin'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2008-02-28.04:47:38.883>
closer = 'jyasskin'
components = ['Library (Lib)']
creation = <Date 2008-01-08.16:39:08.868>
creator = 'jyasskin'
dependencies = []
files = ['9107', '9442']
hgrepos = []
issue_num = 1762
keywords = []
message_count = 14.0
messages = ['59543', '59544', '59545', '59546', '59548', '59550', '59552', '62238', '62240', '62246', '62367', '62382', '62479', '63088']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'nnorwitz', 'rhettinger', 'christian.heimes', 'jyasskin']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1762'
versions = ['Python 2.6']

[jyasskin]

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.

[tiran]

Is this for 2.6 or 3.0?

[jyasskin]

I've only verified the behavior on 2.6, but I suspect it's true for both.

[tiran]

__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__)

[gvanrossum]

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.

[tiran]

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.

[tiran]

The patch implements the rich cmp slots for <, <=, > and >= required for
numbers.Real.

[jyasskin]

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

[rhettinger]

Whoa! I thought we had arrived at a decision to leave decimal alone.
Please do not infect this module with numbers.py.

[jyasskin]

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.

[jyasskin]

I've committed the inlined option as r60762.

[jyasskin]

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.

[jyasskin]

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.

[jyasskin]

Since there were no comments, I submitted the patch as r61098. I think
we're done here. :)