Reimplement ETS to support bag and duplicate_bag

[mfurga]

This PR introduces the following changes:

• Reimplement ETS, replacing the hashtable with a new multimap implementation that supports set, bag, and duplicate_bag table types
• Add insert_new/2, member/2, take/2, update_element/3, update_element/4, lookup_element/4, and delete_object/2 nifs
• Expand the ETS tests for the new table types and functions

Multimap details:

typedef struct EtsMultimapNode
{
    struct EtsMultimapNode *next;
    struct EtsMultimapEntry *entries;
} EtsMultimapNode;

typedef struct EtsMultimapEntry
{
    struct EtsMultimapEntry *next;
    term tuple;
    Heap *heap;
} EtsMultimapEntry;

A multimap can be one of the following types:

• EtsMultimapTypeSingle - each node contains only a single entry, which is basically a regular hashmap. This type is used for set tables.
• EtsMultimapTypeSet - a node can contain multiple entries (no duplicates). This is used for bag tables.
• EtsMultimapTypeList - a node can contain multiple entries (duplicates allowed). This is used for duplicate_bag tables.

Collisions are handled using chaining (each node has a next pointer to the next one). Rehashing is not supported at the moment (the buckets array has a fixed size), so lookup time isn't guaranteed to be constant when there are lots of elements.

Each node must contain at least one entry and always corresponds to exactly one key. If all entries are removed, the node itself is removed as well. All entries in a node have the same key. The key for a node is taken from the first entry and calculated by node_key() as node->entries->tuple[multimap->key_index]. All tuples stored in the multimap use the same index to indicate the key position in the tuple.

New entries are always inserted at the head of the node's entry list. Because allocation can fail during insertion, any entries added so far have to be rolled back. This is handled by insert_revert(), which iterates over all entries in the multimap, compares them by pointer, and removes the matching ones. insert_revert() cannot fail. Example for the EtsMultimapTypeSet type:

A multimap of type EtsMultimapTypeList behaves exactly like EtsMultimapTypeSet, except that duplicate tuples are allowed.

For EtsMultimapTypeSingle, similarly to EtsMultimapTypeSet, all new entries are first inserted at the head of the list. If insertion finishes without errors, the multimap is converted to type EtsMultimapTypeSingle using multimap_to_single(), which keeps only the first entry per node and removes the rest. If an error occurs, insert_revert() is called. multimap_to_single() cannot fail. Example for the EtsMultimapTypeSingle type:

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

[bettio]

Before reviewing the code, I will focus on issues that CI has reported.

1. DCO found sign off is missing, each commit should have at the end "Signed-off-by: Author Name authoremail@example.com". You can fix this with interactive rebase + git commit --amend --signoff

2. There are some minior formatting issues:
   clang-format -i src/libAtomVM/ets.c is enough for fixing them on the C part.
   erlfmt (https://github.com/WhatsApp/erlfmt) should be used on erlang sources: [warn] ./tests/erlang_tests/test_ets.erl

3. You might fix 1 + 2 + 3 together, suggested workflow:

a) git rebase -i main
b) change all commits from pick to edit:

edit c8f87f91 # Add tests for insert_new, bag, and duplicate_bag
edit aa471c07 # Replace ets_hashtable with ets_multimap supporting set, bag, and duplicate_bag
[...]

c) run clang-format and erlfmt (with rebar3) on each updated file, so you clean up history from white space errors and we keep diffs clean.

d) git add -p && git commit --amend --signoff. This allows you also to update commit descriptions etc...

If I may add another advice, I really appreciate how you kept a clean history, but I think we can streamline it a little bit. So before starting with step a) you might consider an initial git rebase -i main:

Each tests + implementation commit pair can be squashed together, into a single commit that contain both tests and implementation. This kind of approach has great benefits if anytime we do a git bisect or we get back in history for tracking any kind of issue, since by doing this ./tests/test-erlang should be always able to pass at each step in git history.

You can do this by using git rebase -i main and by doing a similar rebase plan:

pick c8f87f91 # Add tests for insert_new, bag, and duplicate_bag
pick aa471c07 # Replace ets_hashtable with ets_multimap supporting set, bag, and duplicate_bag
pick 456c0b39 # Add tests for member/2
squash f8d9ac97 # Implement member/2
pick b482dd2d # Add tests for take/2
squash 0291720c # Implement take/2
pick 4d371a40 # Add tests for update_element
squash 995ba867 # Implement update_element

This keeps everything divided in a number of steps, but fewer steps.

So briefly:

First squash together commits and streamline history, after run again git rebase and edit each commit for sign off and white space errors.

If you want to know more about our git usage & style guidelines, you can take a look here: https://github.com/bettio/AtomVM-fork/blob/2a94fa0bafb12826ed20a5e0ebeaed21db9dd37f/GIT_WORKFLOW_AND_HISTORY_STYLE_GUIDE.md

4. Tests are failing against all OTP version <= 26:

 {undef,[{ets,update_element,
             [#Ref<0.2858588063.3122528257.226650>,key_not_exist,
              {2,new_value1},
              {key,value1,value2}],
             []},
        {test_ets,test_update_element,0,
                  [{file,"/__w/AtomVM/AtomVM/tests/erlang_tests/test_ets.erl"},
                   {line,394}]},
        {test_ets,'-isolated/1-fun-0-',3,
                  [{file,"/__w/AtomVM/AtomVM/tests/erlang_tests/test_ets.erl"},
                   {line,753}]}]}

You can use something similar:

get_otp_version() ->
    case erlang:system_info(machine) of
        "BEAM" -> list_to_integer(erlang:system_info(otp_release));
        _ -> atomvm
    end.

and then something like Version when Version =:= atomvm orelse (is_integer(Version) andalso Version >= 24).
You should apply this when testing a feature that appeared only in a recent OTP version.

5. GCC 12 is reporting the following error:

/home/runner/work/AtomVM/AtomVM/src/libAtomVM/ets_multimap.c: In function ‘ets_multimap_remove_tuple’:
/home/runner/work/AtomVM/AtomVM/src/libAtomVM/ets_multimap.c:299:21: error: pointer ‘to_remove’ may be used after ‘realloc’ [-Werror=use-after-free]
  299 |                     free(to_remove);
      |                     ^~~~~~~~~~~~~~~
/home/runner/work/AtomVM/AtomVM/src/libAtomVM/ets_multimap.c:297:52: note: call to ‘realloc’ here
  297 |                 EtsMultimapEntry **new_to_remove = realloc(to_remove, sizeof(EtsMultimapEntry *) * capacity);
      |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

This is a know issue, you can fix with:

// GCC 12 is raising here a false positive warning, according to man realloc:
// "If realloc() fails, the original block is left untouched; it is not freed or moved."
#pragma GCC diagnostic push
#if defined(__GNUC__) && !defined(__clang__) && __GNUC__ == 12
#pragma GCC diagnostic ignored "-Wuse-after-free"
#endif
        free(to_remove);
#pragma GCC diagnostic pop

/__w/AtomVM/AtomVM/src/libAtomVM/ets.c: In function 'ets_delete_table':
/__w/AtomVM/AtomVM/src/libAtomVM/ets.c:517:21: error: statement with no effect [-Werror=unused-value]
  517 |     synclist_wrlock(&ctx->global->ets.ets_tables);
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/AtomVM/AtomVM/src/libAtomVM/synclist.h:128:31: note: in definition of macro 'synclist_wrlock'
  128 | #define synclist_wrlock(list) list
      |                               ^~~~

This happens because the general pattern is like:

struct ListHead *remote_monitors = synclist_wrlock(&conn_obj->remote_monitors);

Those functions are defined like:

ListHead *synclist_wrlock(struct SyncList *synclist);

So you should at least do:

ListHead *locked_list = synclist_wrlock(...);
UNUSED(locked_list)

or maybe (void) synclist_wrlock(...); should be enough.

Still this pattern is a bit suspicious, so I suggest to check if it is 100% legit or if there is any code smell around.

[bettio]

I have a first round of comments.

[bettio]

we have UNREACHABLE()

[mfurga]

The current nifs for ets use AVM_ABORT() instead of UNREACHABLE(); isn't abort a better fit here?

[bettio]

Why did we change this?

[mfurga]

Same as above.

[bettio]

term_is_pos_int applies also here.

[mfurga]

Changed.

[bettio]

Let's prefix this, to avoid any name clash: ETS_MULTIMAP_NUM_BUCKETS

[mfurga]

Changed.

[bettio]

Implementing static functions inside a header may cause code duplication: every time this header is included same code is compiled again as a private copy.

[mfurga]

I deleted ets_multimap_hash.h and moved the hashing impl into ets_multimap.c since it's only used there.

[bettio]

How to address this issues: you might use git commit --fixup=COMMIT_THAT_YOU_ARE_FIXING, so as soon as you do git rebase -i --autosquash it applies the fixups to the right place in git history.

[petermm]

Codex claims this single issue:

1. [P1] ets:update_counter/4 skips default insertion for empty operation lists
   • File: src/libAtomVM/ets.c
   • In ets_update_counter_maybe_gc, the num_ops == 0 branch returns [] and jumps to cleanup before the insert call at src/libAtomVM/ets.c:455.
   • Effect: ets:update_counter(Tab, Key, [], Default) returns [] but does not insert Default when Key is missing.
   • Expected (OTP-aligned) behavior: still insert Default, return [].

So if nothing else add test, so we are confident behaviour is correct.

    % [] with Default object
    S11 = new_table({key, 10, 20, not_number}),
    [] = ets:update_counter(S11, key_not_exist2, [], {key, 10, 20, 30}),
    [{key, 10, 20, not_number}] = ets:lookup(S11, key),
    [{key_not_exist2, 10, 20, 30}] = ets:lookup(S11, key_not_exist2),

[bettio]

I don't think heap should be malloc-ated, I think it should be rather embedded in EtsMultimapEntry, this should both simplify code and increase efficiency.
I don't know how extensive is this change, feel free to decide whether making a new commit or fixing up the commit that introduced ets_multimap.c.

There is an additional improvement we can do here, that is embedding the HeapFragment that holds the storage for that heap. But this specific improvement is beyond the scope of a simple PR and we can just ignore it now and leave a TODO.

Another, more radical, but interesting option would be completely removing EtsMultimapEntry.

typedef struct EtsMultimapNode
{
    Heap heap; // common heap for all entries, memory_heap_append_heap can be used when adding entries.
    struct EtsMultimapNode *next;
    term *entries; //this is a growable array, that is doubled or grown with other strategies. realloc can be used for this purpose.
} EtsMultimapNode;

This would remove the need for malloc() in lookup when returning entries, since they are all available in an array that can be safely returned since table is locked.

Removing an entry means just setting that entry in entries to term_nil(), and once in a while garbage collection might be executed.

Also this approach can be super efficient with set

Still iterating here, the first entry might be stored inside the node itself, avoiding allocation overhead for set and any single-item-nodes.

Anyway let's discuss this.

[bettio]

One more advanced topic: actually we don't even need to keep the Heap struct (and the related bits): the Heap struct is useful when allocating terms, but once this has been done, in our ets case I think the heap can be sealed and we can just throw away the heap.

[mfurga]

I think we can drop the Heap entirely and use a FAM allocation, similar to how it's done in the mailbox:

typedef struct EtsMultimapEntry {
  struct EtsMultimapEntry *next;
  term tuple;
  term storage[]; // storage[0] = mso_list, storage[1...] = term data
} EtsMultimapEntry;

Allocation would look like:

entry = malloc(sizeof(EtsMultimapEntry) + (memory_estimate_usage(tuple) + 1) * sizeof(term));
entry->tuple = memory_copy_term_tree_to_storage(entry->storage, &heap_end, tuple);

This reduces per-entry allocations from 3 to 1, without changing much of the current implementation.

The array approach removes the malloc in lookup but deletion is harder. With a shared heap we can't free memory for a single removed entry. GC would require allocating a new heap, copying all live terms into it and destroying the old one. The same problem applies to insert_revert().

Also lookup callers would need to skip deleted entries (entries with term_nil) which is a bit awkward.

Not sure if the array approach is better here.

[petermm]

Amp delivered this review:
chat here: https://ampcode.com/threads/T-019cc3e7-0bd8-77ec-9566-59e1fe06c597#message-9-block-1 - expanded here: https://gist.github.com/petermm/a800b455b52f85f08d5800d97bf954e8

All caveats apply - and I'm fully pragmatic about llm being both wrong and pedantic - so pick and choose, and let's land this sooner rather than later, even if there is an todo issue - with further hardening..

PR #2157 Code Review — ETS bag/duplicate_bag & New Functions

Branch: pr/2157
Reviewer: AI-assisted (Oracle + manual review)
Date: 2026-03-06
Verdict: ⚠️ Do not merge as-is — several must-fix issues found

Commits Reviewed (8)

#	SHA	Summary
1	ccdf4ce0	Replace ets_hashtable with ets_multimap supporting set, bag, and duplicate_bag
2	80ad60f5	Implement member/2
3	8d00be03	Implement take/2
4	e8cfc243	Implement update_element
5	0682a4ff	Implement delete_object/2
6	52f67345	Add nif stubs
7	958c7e43	Update ETS documentation for bag, duplicate_bag, and new functions
8	96a30340	Update CHANGELOG.md

Files Changed

• src/libAtomVM/ets_multimap.c (new, 747 lines)
• src/libAtomVM/ets_multimap.h (new, 147 lines)
• src/libAtomVM/ets.c (major rewrite, +933/−551)
• src/libAtomVM/ets.h (refactored types/API)
• src/libAtomVM/nifs.c (+372/−177)
• src/libAtomVM/nifs.gperf (new NIF registrations)
• libs/estdlib/src/ets.erl (new exports + docs)
• tests/erlang_tests/test_ets.erl (+711 test lines)
• src/libAtomVM/ets_hashtable.c (deleted)
• src/libAtomVM/ets_hashtable.h (deleted)

---

🔴 Must-Fix (Blockers)

1. SMP Deadlock in ets_delete_table

File: ets.c:504-525

get_table() acquires ETS list rdlock → table wrlock. Then ets_delete_table() acquires the ETS list wrlock while still holding the table lock. This creates a classic lock-ordering deadlock:

• Thread A: holds table wrlock, waits on ETS list wrlock
• Thread B: holds ETS list rdlock, waits on same table lock

Fix: Perform lookup + removal under a single consistent lock order. Acquire the ETS list wrlock first, find/remove the table, then destroy it. Keep lock order globally consistent: list lock → table lock.

---

2. Named Table Creation Race Condition

File: ets.c:134-145, 194

Named table existence is checked under one lock window (synclist_rdlock), but insertion happens later under another (synclist_wrlock in add_table). Two threads can both pass the existence check and create the same named table.

Fix: Perform check + insert under the same ETS list wrlock, or re-check inside add_table() while holding the write lock.

---

3. insert_revert() Corrupts Bucket Chains on OOM

File: ets_multimap.c:394-435

When a reverted node becomes empty, the code does multimap->buckets[i] = next_node; without tracking prev_node. If the emptied node is not the bucket head (hash collision chain), earlier nodes are lost from the bucket chain.

Consequences:

• Silently drops unrelated entries
• Leaks nodes
• Corrupts table state on allocation failure

This is especially serious on ESP32 where OOM is not theoretical.

Fix: Replace the "scan the whole table" rollback with an explicit undo log recording per insertion: target node, inserted entry, and whether the node was newly created. Or at minimum, track prev_node when unlinking empty nodes.

---

4. Hash Correctness Bug: +0.0 vs -0.0

File: ets_multimap.c:326-500

Float hashing uses raw bytes (hash_float), so +0.0 and -0.0 hash to different values. However, term_compare(..., TermCompareExact, ...) treats them as equal via t_float == other_float.

Result: An inserted key {0.0, val} can become unfindable when looked up with -0.0, and vice versa.

Fix:

• Normalize floats before hashing: if f == 0.0, hash as canonical +0.0
• Document that hash must be consistent with the equality semantics used by term_compare

---

5. Signed Integer Overflow UB in Hash Function

File: ets_multimap.c — all hash helpers

All hash accumulators use int32_t h and repeatedly do h = h * LARGE_PRIME + .... In C, signed integer overflow is undefined behavior, not defined wraparound.

Fix: Change the accumulator and all hash helper return types to uint32_t.

---

6. malloc(0) Portability Issue in insert_many()

File: ets.c:684-721

insert_many() does malloc(sizeof(term) * count) even when count == 0. malloc(0) may legally return NULL on some libc/embedded allocators, which would be misinterpreted as EtsAllocationError.

This breaks ets:insert(Tab, []) / ets:insert_new(Tab, []) on some platforms.

Fix: Add early return before the malloc:

if (count == 0) {
    return EtsOk;
}

---

7. Unchecked smp_rwlock_create() Failure

File: ets.c:176-178

Lock allocation is not checked for NULL. On OOM, subsequent SMP_* calls dereference a null pointer.

Fix: Check for NULL and clean up multimap/table on failure.

---

🟡 Important Issues (Should Fix)

8. Incomplete Hashing for External Pid/Port/Ref

File: ets_multimap.c:381-426

External pid/port/ref hashing ignores node and creation fields. While equal terms still compare correctly (no lookup-correctness bug), this causes heavy systematic hash collisions for distributed identities, degrading to O(n) in those buckets.

Fix: Include node identity and creation in the hash computation.

---

9. Unsupported Term Types Silently Degrade

File: ets_multimap.c:738-740

Unsupported term types print to stderr and return the unchanged hash accumulator. This is poor behavior for embedded systems (no stderr) and creates pathological collision behavior.

Fix: Either implement hashing for all term types AtomVM supports in ETS, or reject unsupported keys explicitly instead of silently degrading.

---

10. ordered_set Silently Accepted but Treated as set

File: nifs.c:3789-3799

ordered_set is parsed without error but results in a plain set. Users relying on ordering semantics will get silently wrong behavior.

Fix: Reject ordered_set with an error/warning, or at minimum document this limitation more prominently.

---

11. insert_new/2 Doc/Spec Mismatch for bag Types

File: ets.erl:155-158

Docs say for bag/duplicate_bag it returns false if an identical object already exists. Implementation and tests show stricter behavior: returns false if any object with that key already exists (matching OTP's actual behavior).

Fix: Update docs to match actual behavior.

---

12. Fixed 16-Bucket Hash Table

File: ets_multimap.h:31

With only 16 buckets and the current hash quality issues, performance degrades quickly with table size. Acceptable short-term for embedded, but worth documenting as a known limitation.

---

13. Recursive Hashing Can Blow C Stack on Deep Terms

File: ets_multimap.c:686-741

hash_term_incr recurses into tuples, lists, and maps. On ESP32-class stack sizes (typically 4-8 KB per task), deeply nested keys could cause stack overflow.

Fix (future): Consider iterative hashing or depth limits.

---

✅ Areas That Looked Good

• Memory lifecycle in entry_new/entry_delete/node_delete — no obvious double-free or use-after-free in normal paths
• Heap management in update_element/update_counter with temporary heaps — correctly allocated and destroyed
• Bag/duplicate_bag semantics in happy paths match OTP behavior:
  • bag deduplicates identical objects per key
  • duplicate_bag preserves duplicates
  • delete_object/2 removes all exact duplicates in duplicate_bag
• Test coverage is substantially improved (~711 new lines), covering bag/duplicate_bag across all new operations
• NIF argument validation is generally thorough
• Documentation in ets.erl is clear and well-structured
• Code organization — the refactoring from ets_hashtable to ets_multimap is clean and the API separation between multimap/ets/nifs layers is well-defined

---

Suggested High-Risk Test Scenarios

1. SMP deadlock: One process doing ets:lookup/2 in a loop while another does ets:delete(Tab) repeatedly
2. Named table race: Two processes concurrently calling ets:new(same_name, [named_table])
3. OOM rollback: Inject allocator failures during entry_new/node_new and verify no unrelated entries disappear
4. Float hash correctness: Insert {0.0, v}, lookup with -0.0 (and vice versa)
5. Empty list insert: ets:insert(Tab, []) and ets:insert_new(Tab, []) on target embedded allocator
6. Collision-heavy keys: External pids/refs from different nodes in same table