GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-94673: Add Per-Interpreter Storage for Static Builtin Types by ericsnowcurrently · Pull Request #94995 · python/cpython

Conversation

ericsnowcurrently commented Jul 19, 2022 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This is a precursor to storing tp_subclasses (and tp_weaklist) on the interpreter state for static builtin types. At a high level, we add the following:

_PyStaticType_InitBuiltin()
PyInterpreterState.types.builtins

We also shuffle some code around to be able to use _PyStaticType_InitBuiltin(), especially in Objects/structseq.c.

One thing to note is that we add a new "tp_" field: PyTypeObject.tp_static_builtin_index. If adding another field to PyTypeObject is too costly then we could conditionally re-purpose some other field (e.g. tp_subclasses once we don't use it for static types).

Issue: [subinterpreters] Static types incorrectly share some objects between interpreters #94673

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently added 6 commits

July 18, 2022 20:10


          Add PyInterpreterState.types.

5de3f04


          Add _PyStaticType_InitBuiltin().

24aea28


          Call PyType_Ready() in _PyStaticType_InitBuiltin().

b1cc7d2


          Add _PyStructSequence_InitBuiltin().

c63bccb


          Hard-code _Py_NUM_STATIC_BUILTIN_TYPES.

0aacc7a


          Store per-interpreter state for static builtin types.

a006e21

ericsnowcurrently added the skip issue label

Jul 19, 2022

ericsnowcurrently requested review from iritkatriel and markshannon as code owners

July 19, 2022 02:47

bedevere-bot added the awaiting core review label

Jul 19, 2022

ericsnowcurrently removed request for iritkatriel and markshannon

July 19, 2022 02:47

ericsnowcurrently added skip news and removed skip issue labels

Jul 19, 2022

ericsnowcurrently requested a review from kumaraditya303

July 19, 2022 02:47

ericsnowcurrently commented Jul 19, 2022

Copy link

Copy Markdown

Member Author

For context, I have a branch on top of this one that takes care of tp_subclasses: main...ericsnowcurrently:shareable-static-types-tp_subclasses.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303 commented Jul 19, 2022

Copy link

Copy Markdown

Contributor

Thanks for working on this!

Mostly LGTM, a few changes worth looking into:

The index should be reset after each finalization
Maybe avoid embedding the type cache in type state struct as it may negatively impact performance?
Maybe prefix tp_static_builtin_index with an underscore?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently added 2 commits

July 19, 2022 09:43


          Export _PyStaticType_GetState().

e3ed4fd


          Move _PyStaticType_GetState() down next to _PyStaticType_InitBuiltin().

0c695b5

ericsnowcurrently mentioned this pull request

Jul 19, 2022

[subinterpreters] Static types incorrectly share some objects between interpreters #94673

Closed


          make it compile

2df59c1

ericsnowcurrently commented Jul 19, 2022

Copy link

Copy Markdown

Member Author

Maybe avoid embedding the type cache in type state struct as it may negatively impact performance?

There should be no performance impact since it's still a single static offset from the pointer we were already dereferencing.

Maybe prefix tp_static_builtin_index with an underscore?

None of the other fields have a leading underscore, even the ones that are documented as internal.

That does remind me, I should also add the new field to the docs. (Or maybe I'll ditch the new field and not worry about it. 🙂)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently and others added 2 commits

July 19, 2022 12:13


          Fix a typo.

5d76cfa


          fix test_sys

d05be58

ericsnowcurrently commented

Jul 19, 2022

View reviewed changes

Comment thread

Objects/typeobject.c

		static int
		slot_tp_setattro(PyObject self, PyObject name, PyObject *value);

ericsnowcurrently Jul 19, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently commented Jul 19, 2022

Copy link

Copy Markdown

Member Author

The test_embed failure is because _PyStaticType_Dealloc() bails out if the type still has subclasses (it checks tp_subclasses). This implies that we're still leaking objects out of Py_Finalize(). I'll see what can be done.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently commented Jul 20, 2022

Copy link

Copy Markdown

Member Author

FYI, I tried printing some diagnostic info in _PyStaticType_Dealloc() for types with non-empty tp_subclasses.

minimal:

leaked dict: 1
    13 collections.defaultdict
leaked object: 26
    37 dict
     8 itertools.accumulate
    10 itertools.chain
     9 itertools.combinations
     9 itertools.combinations_with_replacement
     7 itertools.compress
     8 itertools.count
     8 itertools.cycle
     8 itertools.dropwhile
     7 itertools.filterfalse
     8 itertools.groupby
     8 itertools.islice
     6 itertools.pairwise
     9 itertools.permutations
     9 itertools.product
     9 itertools.repeat
     7 itertools.starmap
     8 itertools.takewhile
     8 itertools.zip_longest
     7 itertools._grouper
     8 itertools._tee
     5 itertools._tee_dataobject
    43 collections.deque
     8 _collections._deque_iterator
     8 _collections._deque_reverse_iterator
     9 _collections._tuplegetter

This boils down to static type subclasses in extension modules, since they don't get finalized (ever). Maybe we should ignore static types in tp_subclasses...

a more complex case

./python -E -c 'import sys ; from sysconfig import get_platform ; print("%s-%d.%d" % (get_platform(), *sys.version_info[:2])) (from make -j8):

leaked ZeroDivisionError: 2
     3 DivisionByZero
      (refcount: 2)
     2 DivisionUndefined
      (refcount: 2)
leaked TypeError: 1
     3 FloatOperation
      (refcount: 2)
leaked RuntimeError: 2
     3 RunFailedError
    15 ChannelError
leaked OSError: 2
     3 herror
     3 gaierror
leaked ArithmeticError: 2
     8 ZeroDivisionError
    30 DecimalException
leaked Exception: 12
    22 ArithmeticError
    17 OSError
    14 RuntimeError
     6 TypeError
     4 error
     3 InvalidStateError
     3 ArgumentError
     3 error
     3 RecursingInfinitelyError
     3 error
     3 OSSAudioError
     3 error
leaked BaseException: 2
    58 Exception
     3 CancelledError
leaked tuple: 2
     8 datetime.IsoCalendarDate
     2 DecimalTuple
leaked staticmethod: 1
     4 abstractstaticmethod
leaked property: 1
     4 abstractproperty
leaked list: 2
     3 MyList
     8 xxsubtype.spamlist
leaked dict: 3
    13 collections.defaultdict
     4 StgDict
     6 xxsubtype.spamdict
leaked classmethod: 1
     4 abstractclassmethod
leaked type: 7
    17 ABCMeta
    12 _ctypes.PyCStructType
    12 _ctypes.UnionType
    11 _ctypes.PyCPointerType
    10 _ctypes.PyCArrayType
    10 _ctypes.PyCSimpleType
    10 _ctypes.PyCFuncPtrType
leaked object: 91
    49 type
    13 classmethod
    42 dict
    41 list
    20 property
    14 staticmethod
    29 tuple
    59 BaseException
     9 ModuleSpec
     5 BuiltinImporter
     9 _LoaderBasics
    10 FileLoader
    16 _abc._abc_data
     2 ABC
     6 Iterable
     7 Sized
     6 Container
     8 itertools.accumulate
     9 itertools.combinations
     9 itertools.combinations_with_replacement
     8 itertools.cycle
     8 itertools.dropwhile
     8 itertools.takewhile
     8 itertools.islice
     7 itertools.starmap
    10 itertools.chain
     7 itertools.compress
     7 itertools.filterfalse
     8 itertools.count
     8 itertools.zip_longest
     6 itertools.pairwise
     9 itertools.permutations
     9 itertools.product
     9 itertools.repeat
     8 itertools.groupby
     7 itertools._grouper
     8 itertools._tee
     5 itertools._tee_dataobject
    43 collections.deque
     8 _collections._deque_iterator
     8 _collections._deque_reverse_iterator
     9 _collections._tuplegetter
     4 _IterationGuard
     5 WeakSet
    39 _socket.socket
    17 _struct.Struct
     7 _struct.unpack_iterator
     8 _asyncio.FutureIter
     5 TaskStepMethWrapper
     3 _RunningLoopHolder
    31 _asyncio.Future
     4 CArgObject
     2 _ctypes.CThunkObject
    21 _ctypes._CData
     9 _ctypes.CField
     4 _ctypes.DictRemover
     2 _ctypes.StructParam_Type
    76 _curses.window
    38 datetime.date
    29 datetime.time
    36 datetime.timedelta
    13 datetime.tzinfo
   100 decimal.Decimal
    85 decimal.Context
    21 decimal.SignalDictMixin
     5 decimal.ContextManager
     8 Number
     4 _elementtree._element_iterator
    10 xml.etree.ElementTree.TreeBuilder
    35 xml.etree.ElementTree.Element
    11 xml.etree.ElementTree.XMLParser
    17 _multiprocessing.SemLock
     2 _pickle.Pdata
     8 _pickle.PicklerMemoProxy
     8 _pickle.UnpicklerMemoProxy
    12 _pickle.Pickler
     9 _pickle.Unpickler
     9 matmulType
     4 ipowType
     7 awaitType
     3 GenericAlias
     3 Generic
     9 MethInstance
     9 MethClass
     9 MethStatic
     4 _testcapi.HeapCType
     4 _testcapi.ContainerNoGC
    11 MethodDescriptorBase
    18 _xxsubinterpreters.ChannelID
    26 ossaudiodev.oss_audio_device
    13 ossaudiodev.oss_mixer_device

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently added 3 commits

July 20, 2022 11:11


          Merge branch 'main' into init-static-builtin

1a3d6f1


          Ignore non-builtin static types in _PyStaticType_Dealloc().

160738b


          Remember if a type is a static builtin.

ericsnowcurrently added 10 commits

July 20, 2022 15:28


          Factor out get_static_builtin_index() and set_static_builtin_index().

6820fca


          Revert to skipping on subclasses for now.

98f9eaf


          Throw away the leaking subclasses.

a3fd8e3


          Reset state->type later on.

cb3895c


          Add some asserts.

8d7edd7


          Decrement num_builtins_initialized.

f057fcf


          Factor out subclass_from_ref().

bb8264d


          Handle MemoryError in remove_subclass().

d680e8e


          Call _PyTypes_FiniTypes() before _PyTypes_Fini().

41850e8


          Verify the per-interpreter type state has been cleared.

f1bbbd2

ericsnowcurrently commented Jul 22, 2022

Copy link

Copy Markdown

Member Author

To solve the test_embed failure, I had to stop ignoring types in _PyStaticType_Dealloc() when tp_subclasses was not cleared. This means the subclasses (from non-PEP 489 extension modules) are leaking. However, they were already leaking.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently commented Jul 22, 2022

Copy link

Copy Markdown

Member Author

I'm going to split this PR up.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently closed this

Jul 22, 2022

This was referenced Jul 22, 2022

gh-94673: Add _PyStaticType_InitBuiltin() #95152

Merged

gh-94673: Always Finalize Static Builtin Types #95153

Merged

gh-94673: Add Per-Interpreter Storage for Static Builtin Types #95255

Merged

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

ericsnowcurrently commented Jul 19, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Jul 19, 2022

Uh oh!

kumaraditya303 commented Jul 19, 2022

Uh oh!

ericsnowcurrently commented Jul 19, 2022

Uh oh!

ericsnowcurrently Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented Jul 19, 2022

Uh oh!

ericsnowcurrently commented Jul 20, 2022

Uh oh!

ericsnowcurrently commented Jul 22, 2022

Uh oh!

ericsnowcurrently commented Jul 22, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ericsnowcurrently commented Jul 19, 2022 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.