py/map: Convert map implementation to preserve insertion order. #10
py/map: Convert map implementation to preserve insertion order. #10andrewleech wants to merge 6 commits intomasterfrom
Conversation
TODO: - make it optional at compile-time - make OrderedDict use this new implementation - implement more efficient deletion - profile performance and memory use Signed-off-by: Damien George <damien@micropython.org>
This commit improves the ordered dict implementation from PR micropython#6173: 1. O(1) len() tracking: Add `filled` field to mp_map_t to track actual element count separately from `used` (insertion index). This avoids O(n) scan when computing dict length. 2. Large dict support: Add MICROPY_PY_MAP_LARGE config option to support dicts with >65535 elements using uint32_t hash table indices. 3. Compaction on delete: Add mp_map_compact() that triggers when tombstones exceed 50% of live entries, preventing unbounded memory growth from repeated delete+insert cycles. The table can now shrink when entries are deleted. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Test dict compaction behavior: - dict_compact_order.py: Verify insertion order preserved after compaction - dict_compact_threshold.py: Test threshold boundary and deletion patterns - dict_compact_empty.py: Empty dict edge cases - stress/dict_compact.py: Stress test for memory growth from add/delete Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
- Remove duplicate mp_const_empty_map const (now a macro in obj.h) - Use 'static' instead of 'STATIC' for mp_map_rehash - Add missing .filled = 0 to mp_const_empty_dict_obj
When heap is locked and the dense array is full but contains tombstones, reclaim the tail tombstone to allow adding new entries without allocation. This fixes heap-locked exception handling where Python 3 semantics delete the exception variable after each except block, creating tombstones. Two changes: 1. Treat hash table entries pointing beyond `used` as empty (stale entries) 2. On hash probe wrap-around, if last slot is tombstone, decrement `used` Controlled by MICROPY_PY_MAP_REUSE_TAIL_TOMBSTONE (default enabled, ~50 bytes). Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
|
Code size report: |
|
/review |
![mpy-reviewer[bot]](https://avatars.githubusercontent.com/in/2934917?s=60&v=4){kind=small}
There was a problem hiding this comment.
Several blocking issues need addressing before this can land. The most critical are: an out-of-bounds read in mp_map_init_copy that ASAN catches on dict_fixed.py, a memory leak from m_del being called with the wrong byte size on every map dealloc/rehash, and heap-locked allocation failures in settrace_stackless (heapalloc.py, extreme_exc.py, heapalloc_super.py crash with MemoryError: memory allocation failed, heap is locked). DEVLOG.md should not be committed to the repository. Also missing: a MICROPY_PY_MAP_ORDERED compile-time toggle to select between implementations (noted as TODO in the DEVLOG itself).
CI Issues: sanitize_address fails on basics/dict_fixed.py (out-of-bounds read in mp_map_init_copy). settrace_stackless fails on micropython/heapalloc.py, micropython/extreme_exc.py, and micropython/heapalloc_super.py — all crash with MemoryError: memory allocation failed, heap is locked, indicating the new implementation triggers heap allocation during previously allocation-free operations.
| @@ -0,0 +1,314 @@ | |||
| # MicroPython Ordered Dict Development Log | |||
There was a problem hiding this comment.
Don't commit this to the repo.
| map->is_ordered = src->is_ordered; | ||
| size_t n = MP_MAP_TABLE_BYTE_SIZE(map->alloc); | ||
| map->table = m_malloc0(n); | ||
| memcpy(map->table, src->table, n); |
There was a problem hiding this comment.
src->table for an ordered map (including ROM maps) has only src->alloc * sizeof(mp_map_elem_t) bytes — there are no hash indices appended. Copying MP_MAP_TABLE_BYTE_SIZE(src->alloc) bytes is an out-of-bounds read. This is what ASAN catches in dict_fixed.py. Need:
size_t n = src->is_ordered
? src->alloc * sizeof(mp_map_elem_t)
: MP_MAP_TABLE_BYTE_SIZE(src->alloc);| mp_map_lookup(map, old_table[i].key, MP_MAP_LOOKUP_ADD_IF_NOT_FOUND)->value = old_table[i].value; | ||
| } | ||
| } | ||
| m_del(mp_map_elem_t, old_table, old_alloc); |
There was a problem hiding this comment.
m_del(mp_map_elem_t, old_table, old_alloc) frees old_alloc * sizeof(mp_map_elem_t) bytes, but the allocation was MP_MAP_TABLE_BYTE_SIZE(old_alloc) bytes. Memory leak of MP_MAP_INDEX_SIZE(old_alloc) * old_alloc bytes on every rehash. Same bug in mp_map_compact and in mp_map_deinit (which uses the pre-existing m_del call that now also has the wrong size). Need m_free(old_table, MP_MAP_TABLE_BYTE_SIZE(old_alloc)) at all three sites.
| // Skip if dict is now empty (no point compacting an empty dict). | ||
| if (map->filled > 0 && map->used - map->filled > map->filled / 2) { | ||
| mp_map_compact(map); | ||
| // After compact, original slot is invalid; return value in first empty slot |
There was a problem hiding this comment.
After mp_map_compact(), map->used == map->filled and &map->table[map->used] is the next insertion slot. Storing the deleted value there with key = MP_OBJ_NULL is fragile — any reentrant lookup that inserts will clobber that slot before the caller reads slot->value. Pass the value out via an out-param or save it before calling compact.
| void mp_map_init(mp_map_t *map, size_t n); | ||
| void mp_map_init_fixed_table(mp_map_t *map, size_t n, const mp_obj_t *table); | ||
| void mp_map_init_copy(mp_map_t *map, const mp_map_t *src); | ||
| mp_map_t *mp_map_new(size_t n); |
There was a problem hiding this comment.
mp_map_new and mp_map_free are declared here but not defined anywhere in this diff.
| // Macros and functions to deal with key/value table and hash table. | ||
| // map->table points to the key/value table, then the hash table follows, | ||
| // which can be uint8_t, uint16_t, or uint32_t depending on allocation size. | ||
| #define MP_MAP_IS_UINT8(alloc) ((alloc) < 255) |
There was a problem hiding this comment.
Should be < 256. With 1-based indexing, alloc=255 stores max index 255 which fits in uint8_t.
| #define MP_MAP_IS_UINT8(alloc) ((alloc) < 255) | |
| #define MP_MAP_IS_UINT8(alloc) ((alloc) < 256) |
|
|
||
| // Whether to support dicts with >65535 elements (requires uint32_t hash indices) | ||
| #ifndef MICROPY_PY_MAP_LARGE | ||
| #define MICROPY_PY_MAP_LARGE (1) |
There was a problem hiding this comment.
Should default to 0 — most embedded targets won't need >65535-element dicts, and enabling this by default adds the uint32_t index paths to all builds.
| size_t used : (8 * sizeof(size_t) - 3); | ||
| size_t used : (8 * sizeof(size_t) - 3); // next insertion index in dense array | ||
| size_t alloc; | ||
| size_t filled; // number of non-deleted entries (for O(1) len()) |
There was a problem hiding this comment.
Adding a full size_t filled costs +4 or +8 bytes per mp_map_t. Every module dict, class dict, and instance dict pays this. Is there a way to track it more cheaply, e.g., derive it from used minus the tombstone count?
3 participants
Summary
py/map: Convert map implementation to preserve insertion order.
Testing
TBD
Trade-offs and Alternatives
TBD