gh-142889: Address PR review comments on PyDictKeysObject restructure

  replace char dk_indices[] with explicit union named dk_entries
  because union has non-zero size, update allocation and size reporting
  to use offsetof in place of sizeof, keeping actual memory footprint unchanged
  remove _DK_INDICES_END(), inline uses in macros
  adds PyDictKeysObject pointer note to layout diagram for dictobject.c

Author: clintonsteiner

Include/internal/pycore_dict.h

@@ -198,29 +198,13 @@ struct _dictkeysobject {
     /* Number of used entries in dk_entries. */
     Py_ssize_t dk_nentries;
 
-    /* Offset to entries within this allocation.
-
-       PyDictKeysObject * points to dk_refcnt.  The actual hash table
-       (dk_indices) is stored immediately before the struct in memory;
-       see _DK_INDICES_END() and _DK_INDICES_BASE().
-
-       dk_indices marks the start of the entries array and is used by
-       DK_ENTRIES() / DK_UNICODE_ENTRIES(). */
-    char dk_indices[];  /* char is required to avoid strict aliasing. */
-
-    /* dk_indices is the actual hash table of dk_size entries. It holds
-       indices in dk_entries, or DKIX_EMPTY(-1) or DKIX_DUMMY(-2).
-
-       Indices must be: 0 <= indice < USABLE_FRACTION(dk_size).
-
-       The size in bytes of an indice depends on dk_size:
-
-       - 1 byte if dk_size <= 0xff (char*)
-       - 2 bytes if dk_size <= 0xffff (int16_t*)
-       - 4 bytes if dk_size <= 0xffffffff (int32_t*)
-       - 8 bytes otherwise (int64_t*)
-
-       Dynamically sized, SIZEOF_VOID_P is minimum. */
+    /* The actual hash table (dk_indices) is stored immediately before this
+       struct in memory (negative offsets from dk); see _DK_INDICES_BASE().
+       The entries array is stored here, at the end of the struct. */
+    union {
+        PyDictKeyEntry entries[1];
+        PyDictUnicodeEntry unicode_entries[1];
+    } dk_entries;
 };
 
 /* This must be no more than 250, for the prefix size to fit in one byte. */
@@ -248,10 +232,6 @@ struct _dictvalues {
 #define DK_SIZE(dk)      (1<<DK_LOG_SIZE(dk))
 #endif
 
-static inline void* _DK_INDICES_END(const PyDictKeysObject *dk) {
-    return (void *)dk;
-}
-
 static inline void* _DK_INDICES_BASE(const PyDictKeysObject *dk) {
     size_t indices_size = (size_t)1 << dk->dk_log2_index_bytes;
     return (char *)dk - indices_size;
@@ -262,16 +242,17 @@ static inline void* _DK_ALLOC_BASE(PyDictKeysObject *dk) {
 }
 
 static inline void* _DK_ENTRIES(PyDictKeysObject *dk) {
-    return (void *)(&dk->dk_indices[0]);
+    return (void *)(&dk->dk_entries);
 }
 
 static inline PyDictKeyEntry* DK_ENTRIES(PyDictKeysObject *dk) {
     assert(dk->dk_kind == DICT_KEYS_GENERAL);
-    return (PyDictKeyEntry*)_DK_ENTRIES(dk);
+    return dk->dk_entries.entries;
 }
+
 static inline PyDictUnicodeEntry* DK_UNICODE_ENTRIES(PyDictKeysObject *dk) {
     assert(dk->dk_kind != DICT_KEYS_GENERAL);
-    return (PyDictUnicodeEntry*)_DK_ENTRIES(dk);
+    return dk->dk_entries.unicode_entries;
 }
 
 #define DK_IS_UNICODE(dk) ((dk)->dk_kind != DICT_KEYS_GENERAL)

Modules/_testinternalcapi.c

@@ -1942,12 +1942,12 @@ dict_check_indices_layout(PyObject *self, PyObject *arg)
 
     bool ok = true;
     ok &= (header == base + indices_size);
-    ok &= (entries == header + sizeof(PyDictKeysObject));
+    ok &= (entries == header + offsetof(PyDictKeysObject, dk_entries));
 
     size_t index_bytes = dict_index_bytes_for_keys(keys);
     char *idx_base = (char *)_DK_INDICES_BASE(keys);
     /* Index 0 is stored immediately before the header. */
-    char *idx0 = (char *)_DK_INDICES_END(keys) - (ptrdiff_t)index_bytes;
+    char *idx0 = (char *)keys - (ptrdiff_t)index_bytes;
     ok &= (idx0 == idx_base + indices_size - (ptrdiff_t)index_bytes);
 
     return PyBool_FromLong(ok);

Objects/dictobject.c

@@ -19,7 +19,7 @@ As of Python 3.6, this is compact and ordered. Basic idea is described here:
 +---------------------+
 | dk_indices[]        |
 |                     |
-+---------------------+
++---------------------+         <---- PyDictKeysObject* pointer points here
 | dk_refcnt           |
 | dk_log2_size        |
 | dk_log2_index_bytes |
@@ -182,8 +182,8 @@ ASSERT_DICT_LOCKED(PyObject *op)
 
 #define IS_DICT_SHARED(mp) _PyObject_GC_IS_SHARED(mp)
 #define SET_DICT_SHARED(mp) _PyObject_GC_SET_SHARED(mp)
-#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size##_relaxed(&((const int##size##_t*)_DK_INDICES_END(keys))[-1 - (idx)]);
-#define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size##_relaxed(&((int##size##_t*)_DK_INDICES_END(keys))[-1 - (idx)], (int##size##_t)value);
+#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size##_relaxed(&((const int##size##_t*)(keys))[-1 - (idx)]);
+#define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size##_relaxed(&((int##size##_t*)(keys))[-1 - (idx)], (int##size##_t)value);
 #define ASSERT_OWNED_OR_SHARED(mp) \
     assert(_Py_IsOwnedByCurrentThread((PyObject *)mp) || IS_DICT_SHARED(mp));
 
@@ -262,8 +262,8 @@ static inline void split_keys_entry_added(PyDictKeysObject *keys)
 #define UNLOCK_KEYS_IF_SPLIT(keys, kind)
 #define IS_DICT_SHARED(mp) (false)
 #define SET_DICT_SHARED(mp)
-#define LOAD_INDEX(keys, size, idx) ((const int##size##_t*)_DK_INDICES_END(keys))[-1 - (idx)]
-#define STORE_INDEX(keys, size, idx, value) ((int##size##_t*)_DK_INDICES_END(keys))[-1 - (idx)] = (int##size##_t)value
+#define LOAD_INDEX(keys, size, idx) ((const int##size##_t*)(keys))[-1 - (idx)]
+#define STORE_INDEX(keys, size, idx, value) ((int##size##_t*)(keys))[-1 - (idx)] = (int##size##_t)value
 
 static inline void split_keys_entry_added(PyDictKeysObject *keys)
 {
@@ -651,7 +651,7 @@ static const _PyDict_EmptyKeysStorage empty_keys_storage = {
         1, /* dk_version */
         0, /* dk_usable (immutable) */
         0, /* dk_nentries */
-        {},
+        {}, /* dk_entries */
     }
 };
 
@@ -836,7 +836,7 @@ new_keys_object(uint8_t log2_size, bool unicode)
 
     if (base == NULL) {
         base = PyMem_Malloc(indices_size
-                            + sizeof(PyDictKeysObject)
+                            + offsetof(PyDictKeysObject, dk_entries)
                             + entry_size * usable);
         if (base == NULL) {
             PyErr_NoMemory();
@@ -859,7 +859,7 @@ new_keys_object(uint8_t log2_size, bool unicode)
     dk->dk_usable = usable;
     dk->dk_version = 0;
     memset(_DK_INDICES_BASE(dk), 0xff, indices_size);
-    memset(&dk->dk_indices[0], 0, entry_size * usable);
+    memset(&dk->dk_entries, 0, entry_size * usable);
     return dk;
 }
 
@@ -4921,7 +4921,7 @@ _PyDict_KeysSize(PyDictKeysObject *keys)
 {
     size_t es = (keys->dk_kind == DICT_KEYS_GENERAL
                  ? sizeof(PyDictKeyEntry) : sizeof(PyDictUnicodeEntry));
-    size_t size = sizeof(PyDictKeysObject);
+    size_t size = offsetof(PyDictKeysObject, dk_entries);
     size += (size_t)1 << keys->dk_log2_index_bytes;
     size += USABLE_FRACTION((size_t)DK_SIZE(keys)) * es;
     return size;