cpython icon indicating copy to clipboard operation
cpython copied to clipboard

Support the use of the managed pre-header in builtin classes.

Open markshannon opened this issue 3 years ago • 2 comments

Currently Py_TPFLAGS_MANAGED_DICT is an internal-only flag, in fact setting in third-party code is likely to lead to a crash. We would like to expose it, and a weakref equivalent Py_TPFLAGS_MANAGED_WEAKREFS to allow builtin classes to take advantage of compact object layout. Compact layout uses less memory, performs better and allow more robust subclassing. So everyone should be able to use it.

  • [x] Fix crashes when Py_TPFLAGS_MANAGED_DICT is used
  • [x] Add Py_TPFLAGS_MANAGED_WEAKREFS
  • [ ] Document how to use these flags, and the upgrade path from tp_dictoffset and tp_weakreflist

markshannon avatar Aug 05 '22 11:08 markshannon

The changeset broke wasm32-emscripten builds.

$ ./Tools/wasm/wasm_build.py emscripten-node-dl-debug
$ cd builddir/emscripten-node-dl-debug/
$ $ make test TESTOPTS="test_capi"
...
0:00:03 load avg: 3.51 [1/1/1] test_capi crashed (Exit code 1)
warning: unsupported syscall: __syscall_prlimit64

exiting due to exception: RuntimeError: null function or function signature mismatch,RuntimeError: null function or function signature mismatch
    at gc_collect_main (wasm://wasm/02f6208e:wasm-function[4421]:0x321118)
    at _PyObject_GC_Link (wasm://wasm/02f6208e:wasm-function[4431]:0x321c49)
    at _PyObject_GC_New (wasm://wasm/02f6208e:wasm-function[4432]:0x321d0b)
    at wrapperdescr_get (wasm://wasm/02f6208e:wasm-function[1036]:0x1aeaa2)
    at super_getattro (wasm://wasm/02f6208e:wasm-function[2415]:0x2157fc)
    at PyObject_GetAttr (wasm://wasm/02f6208e:wasm-function[2060]:0x1fbb08)
    at _PyEval_EvalFrameDefault (wasm://wasm/02f6208e:wasm-function[3281]:0x2a7e37)
    at _PyEval_Vector (wasm://wasm/02f6208e:wasm-function[3282]:0x2a9185)
    at _PyFunction_Vectorcall (wasm://wasm/02f6208e:wasm-function[870]:0x1a2a57)
    at _PyObject_FastCallDictTstate (wasm://wasm/02f6208e:wasm-function[857]:0x1a1f22)

== Tests result: FAILURE ==

tiran avatar Aug 15 '22 12:08 tiran

The clear function signature is incorrect. The PR defines it as static void heapmanaged_clear but it must be static int heapmanaged_clear.

I'm also getting warnings on X86_64:

comparison of integer expressions of different signedness: ‘Py_ssize_t’ {aka ‘long int’} and ‘long unsigned int’ [-Wsign-compare]

tiran avatar Aug 15 '22 12:08 tiran

Ping @markshannon -- this still is not documented (there are some mentions of the flags but no explanations or documentation for the upgrade path). This came up in GH-104795.

gvanrossum avatar May 23 '23 20:05 gvanrossum

@gvanrossum, see also #107073. Seems like Py_TPFLAGS_MANAGED_DICT depends on private API.

erlend-aasland avatar Sep 15 '23 12:09 erlend-aasland

It’s public enough unless Victor moves it. :-)

gvanrossum avatar Sep 15 '23 14:09 gvanrossum

It’s public enough unless Victor moves it. :-)

Yes, it's included in Python.h, but most people (both users and core devs) assume an underscore-prefixed API is a private API, not a public API. Especially non-documented underscore-prefixed APIs ;) So, I'm not sure it is "public enough". At least it should be documented.

erlend-aasland avatar Sep 18 '23 08:09 erlend-aasland

We can't change 3.12 so please document it. And feelings around leading _ are all over the place.

gvanrossum avatar Sep 18 '23 15:09 gvanrossum