Repository navigation
Empty __slots__ can create untracked reference cycles #86150
Description
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/brandtbucher'
closed_at = <Date 2020-10-15.01:46:42.602>
created_at = <Date 2020-10-09.19:17:48.839>
labels = ['interpreter-core', '3.8', '3.9', '3.10', 'performance']
title = 'Empty __slots__ can create untracked reference cycles'
updated_at = <Date 2020-10-15.15:51:56.002>
user = 'https://github.com/brandtbucher'bugs.python.org fields:
activity = <Date 2020-10-15.15:51:56.002>
actor = 'brandtbucher'
assignee = 'brandtbucher'
closed = True
closed_date = <Date 2020-10-15.01:46:42.602>
closer = 'brandtbucher'
components = ['Interpreter Core']
creation = <Date 2020-10-09.19:17:48.839>
creator = 'brandtbucher'
dependencies = []
files = []
hgrepos = []
issue_num = 41984
keywords = ['patch']
message_count = 16.0
messages = ['378337', '378339', '378356', '378358', '378637', '378638', '378639', '378640', '378641', '378642', '378643', '378648', '378653', '378655', '378661', '378688']
nosy_count = 5.0
nosy_names = ['tim.peters', 'nascheme', 'pablogsal', 'miss-islington', 'brandtbucher']
pr_nums = ['22701', '22702', '22703', '22707']
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue41984'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']
Activity
pablogsal commentedon Oct 10, 2020
IIRC we do skip the GC flags for user-created types only when the subtype is not adding new variables *and* the base class is not a GC class by itself. This includes the case with __slots__ but is not limited to. If I recall correctly, there is a bunch of metaclasses that fall into this category and some other minor things so maybe is not that bad to unconditionally make all user objects tracked.
In any case I think is prudent to run the performance test suite with PGO/LTO + CPU isolation to get an idea. Unfortinately we already have some unwanted 3.9 performance regressions of unknown origin and I would like to not add to it if we can.
brandtbucher commentedon Oct 14, 2020
Using the following patch:
master...brandtbucher:track-all-heap-types
I got the following pyperformance results (with PGO/LTO and CPU isolation, insignificant rows omitted):
2020-10-13_20-04-master-7992579cd27f.json.gz
============================================
Performance version: 1.0.0
Report on Linux-4.4.0-190-generic-x86_64-with-glibc2.23
Number of logical CPUs: 8
Start date: 2020-10-14 07:49:46.738847
End date: 2020-10-14 08:12:58.050557
2020-10-13_20-37-track-all-heap-types-63d8d25867b5.json.gz
==========================================================
Performance version: 1.0.0
Report on Linux-4.4.0-190-generic-x86_64-with-glibc2.23
Number of logical CPUs: 8
Start date: 2020-10-14 08:29:52.009796
End date: 2020-10-14 08:52:49.230214
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| Benchmark | 2020-10-13_20-04-master-7992579cd27f.json.gz | 2020-10-13_20-37-track-all-heap-types-63d8d25867b5.json.gz | Change | Significance |
+=========================+==============================================+============================================================+==============+=======================+
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| chameleon | 13.7 ms | 13.1 ms | 1.04x faster | Significant (t=7.27) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| chaos | 163 ms | 159 ms | 1.03x faster | Significant (t=5.39) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| crypto_pyaes | 160 ms | 156 ms | 1.02x faster | Significant (t=7.29) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| deltablue | 10.9 ms | 10.7 ms | 1.03x faster | Significant (t=4.36) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| django_template | 73.1 ms | 71.2 ms | 1.03x faster | Significant (t=4.42) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| go | 373 ms | 381 ms | 1.02x slower | Significant (t=-4.71) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| nbody | 206 ms | 200 ms | 1.03x faster | Significant (t=7.52) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| regex_dna | 295 ms | 288 ms | 1.02x faster | Significant (t=5.26) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| richards | 103 ms | 101 ms | 1.03x faster | Significant (t=5.40) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| scimark_sparse_mat_mult | 6.96 ms | 7.17 ms | 1.03x slower | Significant (t=-4.93) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| spectral_norm | 218 ms | 207 ms | 1.05x faster | Significant (t=10.29) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
| unpack_sequence | 101 ns | 84.1 ns | 1.20x faster | Significant (t=36.84) |
+-------------------------+----------------------------------------------+------------------------------------------------------------+--------------+-----------------------+
Note that I used pyperformance==1.0.0, since 1.0.1 won't run on 3.10.
In general, it looks like the patch has no real performance impact. The result for unpack_sequence is *extremely* surprising... I'm quite skeptical of it, although it looks like it is one of the more "micro" benchmarks in the suite.
All things considered, I believe we should fix this... I personally view memory leaks as second only to crashes and incorrect behavior in terms of severity.
pablogsal commentedon Oct 14, 2020
Thanks a lot, Brandt for doing the benchmarking!
I think that unpack_sequence is **very** affected by code locality. I was debugging it recently with Linux perf (https://perf.wiki.kernel.org/index.php/Main_Page) and it shows that is extremely affected by L1-L2 cache performance. I still need to understand why, but yeah, I am fine ignoring it.
Yeah, I very much agree with you. Could you convert your patch into a PR?
brandtbucher commentedon Oct 14, 2020
No problem. I just need to rework some hacks in test_finalization where we use empty __slots__ to create non-GC types. I think I can just create a simple utility in _testcapi to untrack instances for this purpose.
pablogsal commentedon Oct 14, 2020
I have the feeling that I am misunderstanding what you refer to, but if is **untrack** what you need, there is gc.untrack()
brandtbucher commentedon Oct 14, 2020
I'm not sure that's true... :)
Although perhaps track()/untrack() functions *could* be useful to add to the gc module... but that's a separate conversation.
pablogsal commentedon Oct 14, 2020
Oh boy, I need to....ehem....yeah I will be back in....one second...
pablogsal commentedon Oct 14, 2020
In any case, you can expose:
https://docs.python.org/3/c-api/gcsupport.html#c.PyObject_GC_UnTrack
in the testcapi module
brandtbucher commentedon Oct 14, 2020
I'll hold off until https://bugs.python.org/issue42039 is resolved.
brandtbucher commentedon Oct 14, 2020
Actually, never mind. Because of the way finalization works, we need to create untracked *types* for these tests, not untracked *instances*.
PR up soon.
brandtbucher commentedon Oct 15, 2020
New changeset c13b847 by Brandt Bucher in branch 'master':
bpo-41984: GC track all user classes (GH-22701)
c13b847
brandtbucher commentedon Oct 15, 2020
New changeset d197b2b by Miss Skeleton (bot) in branch '3.9':
bpo-41984: GC track all user classes (GH-22701/GH-22702)
d197b2b
brandtbucher commentedon Oct 15, 2020
New changeset aeb66c1 by Miss Skeleton (bot) in branch '3.8':
bpo-41984: GC track all user classes (GH-22701/GH-22707)
aeb66c1