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

Optimize ABC caches #383

Merged
merged 8 commits into from Feb 10, 2017

Conversation

@ilevkivskyi
Copy link
Collaborator

commented Feb 1, 2017

Extensive use of isinstance with ABCs causes significant increase in memory consumption by generics, see https://mail.python.org/pipermail/python-dev/2017-January/147194.html

Here I make _abc_cache and friends shortcut to the type's __extra__ or _gorg (using descriptors where necessary). This allows to reduce memory consumption up to 4x on a micro-benchmark simulating conditions described in the python-dev thread. (There is of course a speed penalty, but it seems to be minor).

@methane Please take a look.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 1, 2017

@methane I also inlined internal helpers to speed-up isinstance() up to 2x.

@methane

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

Because abc is full of magic, I can't say LGTM.
But basic idea seems very right. Thanks to fix it.
I'll check startup time and memory consumption.

BTW, this issue is new from 3.5.3 and 3.6.0?
Does Python 3.5.2 have less ABCs when using lot's of typing.List[X] for hundreds of classes?

self._abc_cache = self.__extra__._abc_cache
elif self.__origin__ is not None:
self._abc_registry = self.__origin__._abc_registry
self._abc_cache = self.__origin__._abc_cache

This comment has been minimized.

Copy link
@methane

methane Feb 2, 2017

Member

How about sharing self._abc_negative_cache too?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Feb 2, 2017

Author Collaborator

self._abc_negative_cache is also shared, but it could be overwritten by abc.py on negative cache invalidation, this is why I made it a descriptor 7 lines below. (Positive cache is never invalidated, so that the sharing could be realised as a simple assignment on instantiation).

This comment has been minimized.

Copy link
@methane

methane Feb 2, 2017

Member

tracemalloc shows this.
I think removing _abc_negative_cache at __init__ or __new__ is worth enough.

125.34KiB / count=573
  File "/Users/inada-n/local/py37/lib/python3.7/_weakrefset.py", line 48
    self._iterating = set()
  File "/Users/inada-n/local/py37/lib/python3.7/abc.py", line 147
    cls._abc_negative_cache = WeakSet()
  File "/Users/inada-n/local/py37/lib/python3.7/typing.py", line 125
    return super().__new__(cls, name, bases, namespace)

123.44KiB / count=1193
  File "/Users/inada-n/local/py37/lib/python3.7/_weakrefset.py", line 38
    def _remove(item, selfref=ref(self)):
  File "/Users/inada-n/local/py37/lib/python3.7/abc.py", line 147
    cls._abc_negative_cache = WeakSet()
  File "/Users/inada-n/local/py37/lib/python3.7/typing.py", line 125
    return super().__new__(cls, name, bases, namespace)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Feb 2, 2017

Author Collaborator

I could not just remove them, since some other places expect their presence. However, I found a bug in _abc_negative_cache setters, and I made most assignments to _abc_negative_cache a no-op. Could you please check the memory/speed situation once more with the newest commit in my branch?

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 2, 2017

@methane

BTW, this issue is new from 3.5.3 and 3.6.0?

I think it was always like this. Actually, it was maybe even worse. There were some improvements in 3.5.3 and 3.6.0 due to generic type caches (these are unrelated caches that actually help :-))

@methane

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

Memory consumption goes wrong.

996.90KiB / count=3450
  File "/Users/inada-n/local/py37/lib/python3.7/abc.py", line 133
    cls = super().__new__(mcls, name, bases, namespace)
  File "/Users/inada-n/local/py37/lib/python3.7/typing.py", line 125
    return super().__new__(cls, name, bases, namespace)
  File "/Users/inada-n/local/py37/lib/python3.7/typing.py", line 963
    self = super().__new__(cls, name, bases, namespace, _root=True)
  File "/Users/inada-n/local/py37/lib/python3.7/typing.py", line 1126
    orig_bases=self.__orig_bases__)
  File "/Users/inada-n/local/py37/lib/python3.7/typing.py", line 642
    return cached(*args, **kwds)
@methane

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

I can reproduce above with original typing.py.

It wasn't in https://gist.github.com/methane/3c34f11fb677365a7e92afe73aca24e7
So it may caused by some internal mechanizm.

("", line 488 usually includes memory for
intern dict. Some internal cache affects tracemalloc.)

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 2, 2017

The last trace you posted (with 996.90KiB / count=3450) seems to be unrelated to _abc_caches. This is normal creation of generic classes. It could be a problem with tracemalloc. If necessary, you could clean-up generic type caches:

for c in typing._cleanups:
    c()

I don't know how to clean-up other internal caches.

Could you please compare the master version and my patch both in same conditions "from cold start"?


If you are interested, I am running a "micro-benchmark":

import sys
from typing import *
from collections.abc import Iterable

classes = []
for i in range(1000):
    classes.append(type('C'+str(i), (), {}))
    dummy = List[classes[i]]

for i in range(1000):
    dummy = isinstance(classes[i](), Iterable)

sys._debugmallocstats()

This gives me total memory consumption 5.5MB with my patch vs 48MB for typing/master.

@methane

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 2, 2017

There are no regressions.

OK, thanks! Do you think the PR is satisfactory?
We could think about other possible optimizations later.

@methane

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

It looks nice to me.
But I'm not wizard of metaclass. I'm not good code reviewer of this module.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 2, 2017

@methane
Thank you, it was very helpful to see the benchmark on a large code base.

@ilevkivskyi ilevkivskyi force-pushed the ilevkivskyi:optimize-weakset branch from 149b538 to 0ca5c0b Feb 4, 2017
@gvanrossum

This comment has been minimized.

Copy link
Member

commented Feb 10, 2017

I continue to lack the energy to review/approve this (I can handle only moderately complex reviews ATM). Given that the 3.6.1 candidate is expected in about two weeks (see PEP 494) maybe the best way forward is to merge this (and the coverage PR that depends on it) no, merge everything upstream, and hope for the best? The memory stats look encouraging for sure!

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 10, 2017

OK, let's do this. After merging, I will make additional checks (I do refleak checks from time to time, and run full CPython test suite after importing typing).

@ilevkivskyi ilevkivskyi merged commit 58c86ad into python:master Feb 10, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.