Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-115776: Embed the values array into the object, for "normal" Python objects. #116115

Merged
merged 35 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
d5085db
Inline values work in progress
markshannon Feb 3, 2024
0d88de4
Change layout of PyDictValue to make is suitable for appending to end…
markshannon Feb 4, 2024
d922903
Add copy_values function
markshannon Feb 21, 2024
c03e6cb
Tidy up _PyObject_Init
markshannon Feb 21, 2024
621ea3b
Inline values -- Work in progress
markshannon Feb 21, 2024
67daab5
Get inline values closer to working
markshannon Feb 23, 2024
34c7171
Further fixing
markshannon Feb 23, 2024
c237e79
Remove debug field
markshannon Feb 23, 2024
f80befa
Remove unused function
markshannon Feb 23, 2024
a0c11e4
Merge branch 'main' into inline-values
markshannon Feb 24, 2024
bc1ebc8
Two tweaks
markshannon Feb 24, 2024
0dc68a4
Rename dict-or-values to managed-dict
markshannon Feb 24, 2024
084519c
Update object layout doc
markshannon Feb 24, 2024
3744f32
Add news
markshannon Feb 24, 2024
75ee5a0
Fix a couple of compilation errors on Windows
markshannon Feb 24, 2024
162764c
Update gdb support
markshannon Feb 24, 2024
82ece1b
Specialize LOAD_ATTR for (uninitialized) managed dicts
markshannon Feb 24, 2024
6a762ed
Allow extra allocation in JIT for tests.
markshannon Feb 24, 2024
9dbc8dd
Fix error from merge
markshannon Feb 24, 2024
4a1f7b7
Fix another mis-merge and update generated files
markshannon Feb 24, 2024
09121f9
Fix formatting
markshannon Feb 24, 2024
c384b05
Fix up free threading
markshannon Feb 24, 2024
ee5cf2a
Merge branch 'main' into inline-values
markshannon Feb 24, 2024
005b42b
Remove incorrect assertion
markshannon Feb 24, 2024
48d849e
Merge branch 'main' into inline-values
markshannon Feb 24, 2024
682217c
Remove some debug code
markshannon Feb 24, 2024
0ff1709
Fix off by one error.
markshannon Feb 24, 2024
895a944
Simplify PyObject_ClearManagedDict
markshannon Feb 25, 2024
1b4302a
Merge branch 'main' into inline-values
markshannon Mar 5, 2024
ecd4204
Merge branch 'main' into inline-values
markshannon Mar 26, 2024
c05d01d
Get tests passing for free-threaded build
markshannon Mar 26, 2024
d441d7b
Review comment and compiler warnings
markshannon Mar 27, 2024
3395bc2
Update stats gathering.
markshannon Mar 27, 2024
ccceffb
Better type safety. Fewer downcasts, some more upcasts
markshannon Mar 28, 2024
7700177
Merge branch 'main' into inline-values
markshannon Apr 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions Include/cpython/pystats.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,11 @@ typedef struct _object_stats {
uint64_t frees;
uint64_t to_freelist;
uint64_t from_freelist;
uint64_t new_values;
uint64_t inline_values;
uint64_t dict_materialized_on_request;
uint64_t dict_materialized_new_key;
uint64_t dict_materialized_too_big;
uint64_t dict_materialized_str_subclass;
uint64_t dict_dematerialized;
uint64_t type_cache_hits;
uint64_t type_cache_misses;
uint64_t type_cache_dunder_hits;
Expand Down
5 changes: 4 additions & 1 deletion Include/internal/pycore_code.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,10 @@ typedef struct {
typedef struct {
uint16_t counter;
uint16_t type_version[2];
uint16_t keys_version[2];
union {
uint16_t keys_version[2];
uint16_t dict_offset;
};
uint16_t descr[4];
} _PyLoadMethodCache;

Expand Down
64 changes: 52 additions & 12 deletions Include/internal/pycore_dict.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ extern "C" {

#include "pycore_freelist.h" // _PyFreeListState
#include "pycore_identifier.h" // _Py_Identifier
#include "pycore_object.h" // PyDictOrValues
#include "pycore_object.h" // PyManagedDictPointer

// Unsafe flavor of PyDict_GetItemWithError(): no error checking
extern PyObject* _PyDict_GetItemWithError(PyObject *dp, PyObject *key);
Expand Down Expand Up @@ -181,6 +181,10 @@ struct _dictkeysobject {
* [-1] = prefix size. [-2] = used size. size[-2-n...] = insertion order.
*/
struct _dictvalues {
uint8_t capacity;
uint8_t size;
uint8_t embedded;
uint8_t valid;
PyObject *values[1];
};

Expand All @@ -196,6 +200,7 @@ static inline void* _DK_ENTRIES(PyDictKeysObject *dk) {
size_t index = (size_t)1 << dk->dk_log2_index_bytes;
return (&indices[index]);
}

static inline PyDictKeyEntry* DK_ENTRIES(PyDictKeysObject *dk) {
assert(dk->dk_kind == DICT_KEYS_GENERAL);
return (PyDictKeyEntry*)_DK_ENTRIES(dk);
Expand All @@ -211,9 +216,6 @@ static inline PyDictUnicodeEntry* DK_UNICODE_ENTRIES(PyDictKeysObject *dk) {
#define DICT_WATCHER_MASK ((1 << DICT_MAX_WATCHERS) - 1)
#define DICT_WATCHER_AND_MODIFICATION_MASK ((1 << (DICT_MAX_WATCHERS + DICT_WATCHED_MUTATION_BITS)) - 1)

#define DICT_VALUES_SIZE(values) ((uint8_t *)values)[-1]
#define DICT_VALUES_USED_SIZE(values) ((uint8_t *)values)[-2]

#ifdef Py_GIL_DISABLED
#define DICT_NEXT_VERSION(INTERP) \
(_Py_atomic_add_uint64(&(INTERP)->dict_state.global_version, DICT_VERSION_INCREMENT) + DICT_VERSION_INCREMENT)
Expand Down Expand Up @@ -246,25 +248,63 @@ _PyDict_NotifyEvent(PyInterpreterState *interp,
return DICT_NEXT_VERSION(interp) | (mp->ma_version_tag & DICT_WATCHER_AND_MODIFICATION_MASK);
}

extern PyObject *_PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values);
PyAPI_FUNC(bool) _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv);
extern PyDictObject *_PyObject_MakeDictFromInstanceAttributes(PyObject *obj);

PyAPI_FUNC(PyObject *)_PyDict_FromItems(
PyObject *const *keys, Py_ssize_t keys_offset,
PyObject *const *values, Py_ssize_t values_offset,
Py_ssize_t length);

static inline uint8_t *
get_insertion_order_array(PyDictValues *values)
{
return (uint8_t *)&values->values[values->capacity];
}

static inline void
_PyDictValues_AddToInsertionOrder(PyDictValues *values, Py_ssize_t ix)
{
assert(ix < SHARED_KEYS_MAX_SIZE);
uint8_t *size_ptr = ((uint8_t *)values)-2;
int size = *size_ptr;
assert(size+2 < DICT_VALUES_SIZE(values));
size++;
size_ptr[-size] = (uint8_t)ix;
*size_ptr = size;
int size = values->size;
uint8_t *array = get_insertion_order_array(values);
assert(size < values->capacity);
assert(((uint8_t)ix) == ix);
array[size] = (uint8_t)ix;
values->size = size+1;
}

static inline size_t
shared_keys_usable_size(PyDictKeysObject *keys)
{
#ifdef Py_GIL_DISABLED
// dk_usable will decrease for each instance that is created and each
// value that is added. dk_nentries will increase for each value that
// is added. We want to always return the right value or larger.
// We therefore increase dk_nentries first and we decrease dk_usable
// second, and conversely here we read dk_usable first and dk_entries
// second (to avoid the case where we read entries before the increment
// and read usable after the decrement)
return (size_t)(_Py_atomic_load_ssize_acquire(&keys->dk_usable) +
_Py_atomic_load_ssize_acquire(&keys->dk_nentries));
#else
return (size_t)keys->dk_nentries + (size_t)keys->dk_usable;
#endif
markshannon marked this conversation as resolved.
Show resolved Hide resolved
}

static inline size_t
_PyInlineValuesSize(PyTypeObject *tp)
{
PyDictKeysObject *keys = ((PyHeapTypeObject*)tp)->ht_cached_keys;
assert(keys != NULL);
size_t size = shared_keys_usable_size(keys);
size_t prefix_size = _Py_SIZE_ROUND_UP(size, sizeof(PyObject *));
assert(prefix_size < 256);
return prefix_size + (size + 1) * sizeof(PyObject *);
}

int
_PyDict_DetachFromObject(PyDictObject *dict, PyObject *obj);

#ifdef __cplusplus
}
#endif
Expand Down
48 changes: 13 additions & 35 deletions Include/internal/pycore_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,8 @@ _PyObject_Init(PyObject *op, PyTypeObject *typeobj)
{
assert(op != NULL);
Py_SET_TYPE(op, typeobj);
if (_PyType_HasFeature(typeobj, Py_TPFLAGS_HEAPTYPE)) {
Py_INCREF(typeobj);
}
assert(_PyType_HasFeature(typeobj, Py_TPFLAGS_HEAPTYPE) || _Py_IsImmortal(typeobj));
Copy link
Contributor

Choose a reason for hiding this comment

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

I now hit this assertion in Cython modules. The (static) extension type that we use for generators has tp_alloc = PyType_GenericAlloc and calls it in tp_new():

#6  0x00007ffff7cc2e96 in __GI___assert_fail (assertion=assertion@entry=0x5555559db4e0 "_PyType_HasFeature(typeobj, Py_TPFLAGS_HEAPTYPE) || _Py_IsImmortal(typeobj)", file=file@entry=0x5555559db468 "./Include/internal/pycore_object.h", line=line@entry=268, function=function@entry=0x555555a04ad8 <__PRETTY_FUNCTION__.140> "_PyObject_Init") at ./assert/assert.c:101
#7  0x0000555555774e33 in _PyObject_Init (typeobj=0x7ffff6c26d40 <__pyx_type_8buildenv___pyx_scope_struct__genexpr>, op=0x7ffff4f1e210) at ./Include/internal/pycore_object.h:268
#8  0x000055555577cec3 in _PyObject_Init (typeobj=<optimized out>, op=<optimized out>) at ./Include/object.h:430
#9  _PyType_AllocNoTrack (type=type@entry=0x7ffff6c26d40 <__pyx_type_8buildenv___pyx_scope_struct__genexpr>, nitems=0) at Objects/typeobject.c:1908
#10 0x000055555577cf09 in PyType_GenericAlloc (type=0x7ffff6c26d40 <__pyx_type_8buildenv___pyx_scope_struct__genexpr>, nitems=<optimized out>) at Objects/typeobject.c:1922
#11 0x00007ffff6c08100 in __pyx_tp_new_8buildenv___pyx_scope_struct__genexpr (t=<optimized out>, a=<optimized out>, k=k@entry=0x0) at buildenv.c:2954

Is this assertion simply wrong, or is there anything I can do to avoid it? The "immortal objects" API doesn't seem to be intended for public use.

Copy link
Member Author

Choose a reason for hiding this comment

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

#19474 sets the reference count to 1 for statically allocated objects unless Py_BUILD_CORE is defined.

As a workaround, I'd suggest making these classes immortal and making them immutable if you can.

The "immortal objects" API doesn't seem to be intended for public use.

That does seem to be the case. I don't know why. Want to open an issue for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this new assertion exists? The previous if condition came from a conservative change that intended to avoid refcounting static types but wanted to make sure (heap) type objects are correctly kept alive as long as their objects. Now the code requires all non-heap types to be declared immortal. That seems a rather heavy change in requirements.

Copy link
Member Author

Choose a reason for hiding this comment

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

All non-heap types are immortal. Requiring that they are declared as such seems reasonable, and is probably necessary for memory safety.

I don't think you should need to do so explicitly though, PyObject_HEAD_INIT should do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does #117673 fix the problem for you?

Py_INCREF(typeobj);
_Py_NewReference(op);
}

Expand Down Expand Up @@ -611,8 +610,7 @@ extern PyTypeObject* _PyType_CalculateMetaclass(PyTypeObject *, PyObject *);
extern PyObject* _PyType_GetDocFromInternalDoc(const char *, const char *);
extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const char *, int);

extern int _PyObject_InitializeDict(PyObject *obj);
int _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp);
void _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp);
extern int _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
PyObject *name, PyObject *value);
PyObject * _PyObject_GetInstanceAttribute(PyObject *obj, PyDictValues *values,
Expand All @@ -627,46 +625,26 @@ PyObject * _PyObject_GetInstanceAttribute(PyObject *obj, PyDictValues *values,
#endif

typedef union {
PyObject *dict;
/* Use a char* to generate a warning if directly assigning a PyDictValues */
char *values;
} PyDictOrValues;
PyDictObject *dict;
DinoV marked this conversation as resolved.
Show resolved Hide resolved
} PyManagedDictPointer;

static inline PyDictOrValues *
_PyObject_DictOrValuesPointer(PyObject *obj)
static inline PyManagedDictPointer *
_PyObject_ManagedDictPointer(PyObject *obj)
{
assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
return (PyDictOrValues *)((char *)obj + MANAGED_DICT_OFFSET);
}

static inline int
_PyDictOrValues_IsValues(PyDictOrValues dorv)
{
return ((uintptr_t)dorv.values) & 1;
return (PyManagedDictPointer *)((char *)obj + MANAGED_DICT_OFFSET);
}

static inline PyDictValues *
_PyDictOrValues_GetValues(PyDictOrValues dorv)
{
assert(_PyDictOrValues_IsValues(dorv));
return (PyDictValues *)(dorv.values + 1);
}

static inline PyObject *
_PyDictOrValues_GetDict(PyDictOrValues dorv)
_PyObject_InlineValues(PyObject *obj)
{
assert(!_PyDictOrValues_IsValues(dorv));
return dorv.dict;
}

static inline void
_PyDictOrValues_SetValues(PyDictOrValues *ptr, PyDictValues *values)
{
ptr->values = ((char *)values) - 1;
assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES);
assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
assert(Py_TYPE(obj)->tp_basicsize == sizeof(PyObject));
return (PyDictValues *)((char *)obj + sizeof(PyObject));
}

extern PyObject ** _PyObject_ComputedDictPointer(PyObject *);
extern void _PyObject_FreeInstanceAttributes(PyObject *obj);
extern int _PyObject_IsInstanceDictEmpty(PyObject *);

// Export for 'math' shared extension
Expand Down
6 changes: 3 additions & 3 deletions Include/internal/pycore_opcode_metadata.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Include/internal/pycore_uop_ids.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions Include/internal/pycore_uop_metadata.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion Include/object.h
Original file line number Diff line number Diff line change
Expand Up @@ -629,13 +629,18 @@ given type object has a specified feature.
/* Track types initialized using _PyStaticType_InitBuiltin(). */
#define _Py_TPFLAGS_STATIC_BUILTIN (1 << 1)

/* The values array is placed inline directly after the rest of
* the object. Implies Py_TPFLAGS_HAVE_GC.
*/
#define Py_TPFLAGS_INLINE_VALUES (1 << 2)

/* Placement of weakref pointers are managed by the VM, not by the type.
* The VM will automatically set tp_weaklistoffset.
*/
#define Py_TPFLAGS_MANAGED_WEAKREF (1 << 3)

/* Placement of dict (and values) pointers are managed by the VM, not by the type.
* The VM will automatically set tp_dictoffset.
* The VM will automatically set tp_dictoffset. Implies Py_TPFLAGS_HAVE_GC.
*/
#define Py_TPFLAGS_MANAGED_DICT (1 << 4)

Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_capi/test_mem.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ class C(): pass
self.assertIn(b'MemoryError', out)
*_, count = line.split(b' ')
count = int(count)
self.assertLessEqual(count, i*5)
self.assertGreaterEqual(count, i*5-2)
self.assertLessEqual(count, i*10)
self.assertGreaterEqual(count, i*10-4)


# Py_GIL_DISABLED requires mimalloc (not malloc)
Expand Down