Empty __slots__ can create untracked reference cycles

@tim-one

BPO	41984
Nosy	@tim-one, @nascheme, @pablogsal, @miss-islington, @brandtbucher
PRs	bpo-41984: GC track all user classes #22701 [3.9] bpo-41984: GC track all user classes (GH-22701) #22702 [3.8] bpo-41984: GC track all user classes (GH-22701) #22703 [3.8] bpo-41984: GC track all user classes (GH-22701) #22707

^{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']

Currently, we don't track instances of certain heap types based on the assumption that "no members" == "no reference cycles".

Unfortunately, it's still possible to create untracked reference cycles with one's parents. The following program leaks memory:

while True:
    class C:
        __slots__ = ()
    C.i = C()
    del C

The fix is simple: track all instances of user-defined classes, no exceptions. I'm not sure we were actually getting any real wins from the old behavior anyways.

Thanks for noticing this, Brandt!

The fix is simple: track all instances of user-defined classes, no exceptions

I have the fear that this may increase the time of collections substantially because even if many of them won't have references at all, they still net to be visited and they count towards the time complexity of the algorithm that detects isolated sub-cycles. I mention this because this could affect an unbounded number of instances per type. For example, some of the types that are untracked.

Maybe I am missing something but we could mark them as having GC support unconditionally but still leave them untracking and unconditionally add a tracking call on setattribute.

Maybe I am missing something but we could mark them as having GC support unconditionally but still leave them untracking and unconditionally add a tracking call on setattribute.

Hm, I’m not sure that would be enough. Consider the case of a class that registers its instances in a collection of some sort:

while True:
    class D:
        __slots__ = ()
        registry = []
        def __init__(self):
            self.registry.append(self)
    for i in range(100):
        D() and None  # Suppress REPL output.
    del D

This is probably more common (and problematic) than my example above.

At the same time, I agree that it’s not *ideal* to track all of these objects automatically. Anyone setting __slots__ is probably planning on creating lots of “cheap” instances. If they do accidentally create cycles, though, I feel the memory hit then would be worse than any collection overhead.

I’m just not sure I see a way to fix this without tracking them all.

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.

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.

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.

Yeah, I very much agree with you. Could you convert your patch into a PR?

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.

I think I can just create a simple utility in _testcapi to untrack instances for this purpose.

I have the feeling that I am misunderstanding what you refer to, but if is **untrack** what you need, there is gc.untrack()

there is gc.untrack()

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.

I'm not sure that's true... :)

Oh boy, I need to....ehem....yeah I will be back in....one second...

In any case, you can expose:

https://docs.python.org/3/c-api/gcsupport.html#c.PyObject_GC_UnTrack

in the testcapi module

I'll hold off until https://bugs.python.org/issue42039 is resolved.

Actually, never mind. Because of the way finalization works, we need to create untracked *types* for these tests, not untracked *instances*.

PR up soon.

New changeset c13b847 by Brandt Bucher in branch 'master':
bpo-41984: GC track all user classes (GH-22701)
c13b847

New changeset d197b2b by Miss Skeleton (bot) in branch '3.9':
bpo-41984: GC track all user classes (GH-22701/GH-22702)
d197b2b

New changeset aeb66c1 by Miss Skeleton (bot) in branch '3.8':
bpo-41984: GC track all user classes (GH-22701/GH-22707)
aeb66c1

Navigation Menu

Repository navigation

Empty slots can create untracked reference cycles #86150

1 remaining item

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Empty __slots__ can create untracked reference cycles #86150

Description

Activity

brandtbucher commented on Oct 9, 2020

pablogsal commented on Oct 9, 2020

brandtbucher commented on Oct 9, 2020

1 remaining item

brandtbucher commented on Oct 14, 2020

pablogsal commented on Oct 14, 2020

brandtbucher commented on Oct 14, 2020

pablogsal commented on Oct 14, 2020

brandtbucher commented on Oct 14, 2020

pablogsal commented on Oct 14, 2020

pablogsal commented on Oct 14, 2020

brandtbucher commented on Oct 14, 2020

brandtbucher commented on Oct 14, 2020

brandtbucher commented on Oct 15, 2020

brandtbucher commented on Oct 15, 2020

brandtbucher commented on Oct 15, 2020

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Empty slots can create untracked reference cycles #86150