Interns strings in deep-frozen modules.

See faster-cpython/ideas#218

This speeds up comparison of strings by just comparing their pointers so it is much faster.

New changeset c0a5ebe by Kumar Aditya in branch 'main':
bpo-46430: Intern strings in deep-frozen modules (GH-30683)
c0a5ebe

I noticed that the new interning code lacks proper error reporting. There are only asserts. _PyCode_New() checks the return value of intern_strings.

We discussed that and found that a lot of errors are ignored during interning anyway.

But it's not too late to change if you want to (sending a PR would probably be quicker than arguing :-).

New changeset dee020a by Nikita Sobolev in branch 'main':
Fix sphinx-lint after bpo-31097 and b878b3a (GH-31248)
dee020a

I consider this done so closing it as improving the error handling of interning it out of scope of this issue.

Please leave the ticket open until we have an agreement how to handle the missing error checks.

New changeset c0a5ebe by Kumar Aditya in branch 'main':
bpo-46430: Intern strings in deep-frozen modules (GH-30683)

This change introduced a memory leak at Python exit.

Before:

$ ./python -X showrefcount -c pass
[0 refs, 0 blocks]

After:

$ ./python -X showrefcount -c pass
[0 refs, 344 blocks]

This change introduced a memory leak at Python exit.

It's regression related to bpo-1635741 which I fixed recently:
https://mail.python.org/archives/list/python-dev@python.org/thread/E4C6TDNVDPDNNP73HTGHN5W42LGAE22F/

_PyStaticCode_InternStrings() error handling is based on assert(): that's really bad. It can crash Python (exit with abort()) at the first memory allocation failure. Why not returning -1 on error?

Okay, let's change the error handling. @Kumar, can you handle that?

@victor, the refleak is unrelated to the error handling right? Presumably the leak is imaginary -- the deep-frozen interned strings should be accounted for somehow. @Kumar do you need help investigating this?

Presumably the leak is imaginary

Well, you can check with your favorite memory debugger like Valgrind if you don't trust Python internal memory debugger :-)

Not leaking memory at exit matters when Python is embedded in an application.

I don't think that it's related to the error handling which is stripped in release mode.

Not leaking memory at exit matters when Python is embedded
in an application.

Sure, but "leaks" caused by deep-freezing cannot be solved by freeing up the deep-frozen memory -- the solution must be to update the accounting somewhere.

Where is the existence of Py_None accounted for (since it's statically allocated, or at least used to be)? That's likely where we'd have to do something about the deep-frozen objects.

Sure, but "leaks" caused by deep-freezing cannot be solved by freeing up the deep-frozen memory -- the solution must be to update the accounting somewhere.

Python allocates memory (ex: with PyObject_Malloc()) which is not released at exit. Two examples from Valgrind:

==196803== 50 bytes in 1 blocks are still reachable in loss record 1 of 87
==196803== at 0x484486F: malloc (vg_replace_malloc.c:381)
==196803== by 0x544E29: _PyMem_RawMalloc (obmalloc.c:101)
==196803== by 0x545E0E: PyObject_Malloc (obmalloc.c:700)
==196803== by 0x587159: PyUnicode_New (unicodeobject.c:1448)
==196803== by 0x58C4CF: get_latin1_char (unicodeobject.c:2148)
==196803== by 0x58D7F7: _PyUnicode_FromUCS1 (unicodeobject.c:2450)
==196803== by 0x58E374: PyUnicode_FromKindAndData (unicodeobject.c:2525)
==196803== by 0x69402B: r_object (marshal.c:1150)
==196803== by 0x694295: r_object (marshal.c:1212)
==196803== by 0x694AE0: r_object (marshal.c:1393)
==196803== by 0x694295: r_object (marshal.c:1212)
==196803== by 0x694AA4: r_object (marshal.c:1387)
==196803== by 0x6950F3: read_object (marshal.c:1524)
==196803== by 0x6953E7: PyMarshal_ReadObjectFromString (marshal.c:1641)
==196803== by 0x763223: _Py_Get_Getpath_CodeObject (getpath.c:792)
==196803== by 0x76337F: _PyConfig_InitPathConfig (getpath.c:847)
==196803== by 0x68CD04: config_init_import (initconfig.c:1967)
==196803== by 0x68CE4E: _PyConfig_InitImportConfig (initconfig.c:2000)
==196803== by 0x69E594: init_interp_main (pylifecycle.c:1103)
==196803== by 0x69EBF8: pyinit_main (pylifecycle.c:1216)
==196803== by 0x69EDCE: Py_InitializeFromConfig (pylifecycle.c:1247)
==196803== by 0x6D5346: pymain_init (main.c:67)
==196803== by 0x6D64F8: pymain_main (main.c:692)
==196803== by 0x6D65A1: Py_BytesMain (main.c:725)
==196803== by 0x41D7B5: main (python.c:15)

and

==196803== 3,336 bytes in 60 blocks are still reachable in loss record 87 of 87
==196803== at 0x484486F: malloc (vg_replace_malloc.c:381)
==196803== by 0x544E29: _PyMem_RawMalloc (obmalloc.c:101)
==196803== by 0x545E0E: PyObject_Malloc (obmalloc.c:700)
==196803== by 0x587159: PyUnicode_New (unicodeobject.c:1448)
==196803== by 0x59AEB9: unicode_decode_utf8 (unicodeobject.c:5162)
==196803== by 0x59B5B0: PyUnicode_DecodeUTF8Stateful (unicodeobject.c:5292)
==196803== by 0x58D296: PyUnicode_FromString (unicodeobject.c:2322)
==196803== by 0x5CFFAE: PyUnicode_InternFromString (unicodeobject.c:15650)
==196803== by 0x4E8B61: descr_new (descrobject.c:885)
==196803== by 0x4E8C9B: PyDescr_NewMethod (descrobject.c:934)
==196803== by 0x56694C: type_add_method (typeobject.c:5643)
==196803== by 0x566A92: type_add_methods (typeobject.c:5689)
==196803== by 0x569166: type_ready_fill_dict (typeobject.c:6165)
==196803== by 0x569A02: type_ready (typeobject.c:6421)
==196803== by 0x569B59: PyType_Ready (typeobject.c:6457)
==196803== by 0x544363: _PyTypes_InitTypes (object.c:1952)
==196803== by 0x69D12D: pycore_init_types (pylifecycle.c:704)
==196803== by 0x69D8FC: pycore_interp_init (pylifecycle.c:831)
==196803== by 0x69DC31: pyinit_config (pylifecycle.c:887)
==196803== by 0x69E355: pyinit_core (pylifecycle.c:1050)
==196803== by 0x69ED32: Py_InitializeFromConfig (pylifecycle.c:1240)
==196803== by 0x6D5346: pymain_init (main.c:67)
==196803== by 0x6D64F8: pymain_main (main.c:692)
==196803== by 0x6D65A1: Py_BytesMain (main.c:725)
==196803== by 0x41D7B5: main (python.c:15)

Before the commit c0a5ebe, Python didn't leak this memory.

That's pretty mysterious. The deep-freeze code isn't on the stack for either of those, but allocation of new unicode string objects is. I'm guessing these are somehow leaked by the interning, but I don't follow yet how.

PyUnicode_InternInPlace() in intern_strings() can convert an immortal string to a regular Python strong reference, whereas _PyStaticCode_Dealloc() doesn't bother with clearing co_names, co_consts and co_localsplusnames.

I tried to hack _PyStaticCode_Dealloc() to free strings, but since immortal objects are half-baken today, calling Py_DECREF() does crash.

Py_SETREF() should increase _Py_RefTotal if the old object is immortal and he new object is not.

Maybe this change should be reverted until Eric's PEP-683 "Immortal Objects, Using a Fixed Refcount" is implemented.

I have created a PR to fix the memory leak, See #31549

Okay, let's change the error handling. @Kumar, can you handle that?

How it should be handled? Currently PyUnicode_InternInPlace ignores any errors and does not return it. It would be backwards-incompatible to change that, moreover as I explained in #30683 (comment) intern_strings only check if all the items are strings which will be always true in case of deep-freeze so I don't think anything needs to be changed here. I would be interested to know if I am missing something though.

New changeset 4dc7463 by Kumar Aditya in branch 'main':
bpo-46430: Fix memory leak in interned strings of deep-frozen modules (GH-31549)
4dc7463

I wrote #31555 to make sure that Python doesn't leak at Python exit.

How it should be handled? Currently PyUnicode_InternInPlace ignores any errors and does not return it. It would be backwards-incompatible to change that, moreover as I explained in #30683 (comment) intern_strings only check if all the items are strings which will be always true in case of deep-freeze so I don't think anything needs to be changed here. I would be interested to know if I am missing something though.

The other functions you are calling *do* return errors. You should not ignore those. If any errors are reported the caller can decide what to do (e.g. call Py_FatalError().

commit 0d9b565
Author: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Date: Sat Feb 26 22:05:03 2022 +0530

Propagate errors (however unlikely) from _Py_Deepfreeze_Init() (GH-31596)

The other functions you are calling *do* return errors. You should not ignore those. If any errors are reported the caller can decide what to do (e.g. call Py_FatalError().

PEP-587 introduced PyStatus to Python startup code which let the Py_Initialize() caller to decide how to handle errors ;-) For example, you can open a graphical popup rather than killing the process with SIGABRT (Py_FatalError() behavior) which might be more user friendly :-D Or just log the error.

PEP-587 introduced PyStatus to Python startup code which let the Py_Initialize() caller to decide how to handle errors ;-) For example, you can open a graphical popup rather than killing the process with SIGABRT (Py_FatalError() behavior) which might be more user friendly :-D Or just log the error.

That's up to pycore_interp_init() in pylifecycle.c now. Kumar called _PyStatus_ERR() there, so I think nothing more needs to be done.