Merge pull request #1405 from TheSobkiewicz/thesobkiewicz/nifs/ets/refactor_insert

bettio · bettio · commit 652d60efa700 · 2025-02-14T15:08:44.000+01:00
Add option to insert list in ets:insert, ets:lookup refactor

- Enabled ets:insert/2 to accept lists for bulk insertion.
- Extracted helper functions for ets:lookup/2 and ets:insert/2 that do not apply table locks.

The new helper functions can be utilized in the following ETS operations to reduce code duplication:
- ets:update_element/3
- ets:insert_new/2
- ets:update_counter/3
- ets:update_counter/4
- ets:take/2
- ets:delete_object/2

Every mentioned function will be implemented after merging of this PR.

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
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added `socket:getopt/2`
 - Added `supervisor:terminate_child/2`, `supervisor:restart_child/2` and `supervisor:delete_child/2`
 - Added `esp:partition_read/3`, and documentation for `esp:partition_erase_range/2/3` and `esp:partition_write/3`
+- Added support for list insertion in 'ets:insert/2'.
 
 ### Fixed
 - ESP32: improved sntp sync speed from a cold boot.
diff --git a/libs/estdlib/src/ets.erl b/libs/estdlib/src/ets.erl
@@ -63,7 +63,7 @@ new(_Name, _Options) ->
 %% @doc Insert an entry into an ets table.
 %% @end
 %%-----------------------------------------------------------------------------
--spec insert(Table :: table(), Entry :: tuple()) -> true.
+-spec insert(Table :: table(), Entry :: tuple() | [tuple()]) -> true.
 insert(_Table, _Entry) ->
     erlang:nif_error(undefined).
 
diff --git a/src/libAtomVM/ets.c b/src/libAtomVM/ets.c
@@ -252,58 +252,104 @@ static void ets_delete_all_tables(struct Ets *ets, GlobalContext *global)
     ets_delete_tables_internal(ets, true_pred, NULL, global);
 }
 
-EtsErrorCode ets_insert(term ref, term entry, Context *ctx)
+static EtsErrorCode ets_table_insert(struct EtsTable *ets_table, term entry, Context *ctx)
 {
-    struct EtsTable *ets_table = term_is_atom(ref) ? ets_get_table_by_name(&ctx->global->ets, ref, TableAccessWrite) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(ref), TableAccessWrite);
-    if (ets_table == NULL) {
-        return EtsTableNotFound;
-    }
-
     if (ets_table->access_type != EtsAccessPublic && ets_table->owner_process_id != ctx->process_id) {
-        SMP_UNLOCK(ets_table);
         return EtsPermissionDenied;
     }
 
-    if ((size_t) term_get_tuple_arity(entry) < (ets_table->keypos + 1)) {
-        SMP_UNLOCK(ets_table);
+    size_t keypos = ets_table->keypos;
+
+    if ((size_t) term_get_tuple_arity(entry) < keypos + 1) {
         return EtsBadEntry;
     }
 
-    Heap *heap = malloc(sizeof(Heap));
-    if (IS_NULL_PTR(heap)) {
-        SMP_UNLOCK(ets_table);
+    struct HNode *new_node = ets_hashtable_new_node(entry, keypos);
+    if (IS_NULL_PTR(new_node)) {
         return EtsAllocationFailure;
     }
-    size_t size = (size_t) memory_estimate_usage(entry);
-    if (memory_init_heap(heap, size) != MEMORY_GC_OK) {
-        free(heap);
-        SMP_UNLOCK(ets_table);
+
+    EtsHashtableErrorCode res = ets_hashtable_insert(ets_table->hashtable, new_node, EtsHashtableAllowOverwrite, ctx->global);
+    if (UNLIKELY(res != EtsHashtableOk)) {
         return EtsAllocationFailure;
     }
 
-    term new_entry = memory_copy_term_tree(heap, entry);
-    term key = term_get_tuple_element(new_entry, (int) ets_table->keypos);
+    return EtsOk;
+}
 
-    EtsErrorCode ret = EtsOk;
-    EtsHashtableErrorCode res = ets_hashtable_insert(ets_table->hashtable, key, new_entry, EtsHashtableAllowOverwrite, heap, ctx->global);
-    if (UNLIKELY(res != EtsHashtableOk)) {
-        ret = EtsAllocationFailure;
+static EtsErrorCode ets_table_insert_list(struct EtsTable *ets_table, term list, Context *ctx)
+{
+    if (ets_table->access_type != EtsAccessPublic && ets_table->owner_process_id != ctx->process_id) {
+        return EtsPermissionDenied;
     }
 
-    SMP_UNLOCK(ets_table);
+    term iter = list;
+    size_t size = 0;
 
-    return ret;
+    while (term_is_nonempty_list(iter)) {
+        term tuple = term_get_list_head(iter);
+        iter = term_get_list_tail(iter);
+        if (!term_is_tuple(tuple) || (size_t) term_get_tuple_arity(tuple) < (ets_table->keypos + 1)) {
+            return EtsBadEntry;
+        }
+        ++size;
+    }
+    if (!term_is_nil(iter)) {
+        return EtsBadEntry;
+    }
+
+    struct HNode **nodes = malloc(size * sizeof(struct HNode *));
+    if (IS_NULL_PTR(nodes)) {
+        return EtsAllocationFailure;
+    }
+
+    size_t i = 0;
+    while (term_is_nonempty_list(list)) {
+        term tuple = term_get_list_head(list);
+        nodes[i] = ets_hashtable_new_node(tuple, ets_table->keypos);
+        if (IS_NULL_PTR(nodes[i])) {
+            ets_hashtable_free_node_array(nodes, i, ctx->global);
+            free(nodes);
+            return EtsAllocationFailure;
+        }
+        ++i;
+        list = term_get_list_tail(list);
+    }
+
+    for (size_t i = 0; i < size; ++i) {
+
+        EtsHashtableErrorCode res = ets_hashtable_insert(ets_table->hashtable, nodes[i], EtsHashtableAllowOverwrite, ctx->global);
+        assert(res == EtsHashtableOk);
+    }
+
+    free(nodes);
+    return EtsOk;
 }
 
-EtsErrorCode ets_lookup(term ref, term key, term *ret, Context *ctx)
+EtsErrorCode ets_insert(term name_or_ref, term entry, Context *ctx)
 {
-    struct EtsTable *ets_table = term_is_atom(ref) ? ets_get_table_by_name(&ctx->global->ets, ref, TableAccessRead) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(ref), TableAccessRead);
+    struct EtsTable *ets_table = term_is_atom(name_or_ref) ? ets_get_table_by_name(&ctx->global->ets, name_or_ref, TableAccessWrite) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(name_or_ref), TableAccessWrite);
     if (ets_table == NULL) {
         return EtsTableNotFound;
     }
 
+    EtsErrorCode result;
+    if (term_is_tuple(entry)) {
+        result = ets_table_insert(ets_table, entry, ctx);
+    } else if (term_is_list(entry)) {
+        result = ets_table_insert_list(ets_table, entry, ctx);
+    } else {
+        result = EtsBadEntry;
+    }
+
+    SMP_UNLOCK(ets_table);
+
+    return result;
+}
+
+static EtsErrorCode ets_table_lookup(struct EtsTable *ets_table, term key, term *ret, Context *ctx)
+{
     if (ets_table->access_type == EtsAccessPrivate && ets_table->owner_process_id != ctx->process_id) {
-        SMP_UNLOCK(ets_table);
         return EtsPermissionDenied;
     }
 
@@ -316,24 +362,35 @@ EtsErrorCode ets_lookup(term ref, term key, term *ret, Context *ctx)
         size_t size = (size_t) memory_estimate_usage(res);
         // allocate [object]
         if (UNLIKELY(memory_ensure_free_opt(ctx, size + CONS_SIZE, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {
-            SMP_UNLOCK(ets_table);
             return EtsAllocationFailure;
         }
         term new_res = memory_copy_term_tree(&ctx->heap, res);
         *ret = term_list_prepend(new_res, term_nil(), &ctx->heap);
     }
-    SMP_UNLOCK(ets_table);
 
     return EtsOk;
 }
 
-EtsErrorCode ets_lookup_element(term ref, term key, size_t pos, term *ret, Context *ctx)
+EtsErrorCode ets_lookup(term name_or_ref, term key, term *ret, Context *ctx)
+{
+    struct EtsTable *ets_table = term_is_atom(name_or_ref) ? ets_get_table_by_name(&ctx->global->ets, name_or_ref, TableAccessRead) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(name_or_ref), TableAccessRead);
+    if (ets_table == NULL) {
+        return EtsTableNotFound;
+    }
+
+    EtsErrorCode result = ets_table_lookup(ets_table, key, ret, ctx);
+    SMP_UNLOCK(ets_table);
+
+    return result;
+}
+
+EtsErrorCode ets_lookup_element(term name_or_ref, term key, size_t pos, term *ret, Context *ctx)
 {
     if (UNLIKELY(pos == 0)) {
         return EtsBadPosition;
     }
 
-    struct EtsTable *ets_table = term_is_atom(ref) ? ets_get_table_by_name(&ctx->global->ets, ref, TableAccessRead) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(ref), TableAccessRead);
+    struct EtsTable *ets_table = term_is_atom(name_or_ref) ? ets_get_table_by_name(&ctx->global->ets, name_or_ref, TableAccessRead) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(name_or_ref), TableAccessRead);
     if (ets_table == NULL) {
         return EtsTableNotFound;
     }
@@ -368,9 +425,9 @@ EtsErrorCode ets_lookup_element(term ref, term key, size_t pos, term *ret, Conte
     return EtsOk;
 }
 
-EtsErrorCode ets_delete(term ref, term key, term *ret, Context *ctx)
+EtsErrorCode ets_delete(term name_or_ref, term key, term *ret, Context *ctx)
 {
-    struct EtsTable *ets_table = term_is_atom(ref) ? ets_get_table_by_name(&ctx->global->ets, ref, TableAccessRead) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(ref), TableAccessRead);
+    struct EtsTable *ets_table = term_is_atom(name_or_ref) ? ets_get_table_by_name(&ctx->global->ets, name_or_ref, TableAccessRead) : ets_get_table_by_ref(&ctx->global->ets, term_to_ref_ticks(name_or_ref), TableAccessRead);
     if (ets_table == NULL) {
         return EtsTableNotFound;
     }
diff --git a/src/libAtomVM/ets_hashtable.c b/src/libAtomVM/ets_hashtable.c
@@ -35,7 +35,7 @@ struct HNode
     struct HNode *next;
     term key;
     term entry;
-    Heap *heap;
+    Heap heap;
 };
 
 static uint32_t hash_term(term t, GlobalContext *global);
@@ -53,14 +53,26 @@ struct EtsHashTable *ets_hashtable_new()
     return htable;
 }
 
+static void ets_hashtable_free_node(struct HNode *node, GlobalContext *global)
+{
+    memory_destroy_heap(&node->heap, global);
+    free(node);
+}
+
+void ets_hashtable_free_node_array(struct HNode **allocated, size_t size, GlobalContext *global)
+{
+    for (size_t i = 0; i < size; ++i) {
+        ets_hashtable_free_node(allocated[i], global);
+    }
+}
+
 void ets_hashtable_destroy(struct EtsHashTable *hash_table, GlobalContext *global)
 {
     for (size_t i = 0; i < hash_table->capacity; ++i) {
         struct HNode *node = hash_table->buckets[i];
-        while (node != 0) {
-            memory_destroy_heap(node->heap, global);
+        while (node != NULL) {
             struct HNode *next_node = node->next;
-            free(node);
+            ets_hashtable_free_node(node, global);
             node = next_node;
         }
     }
@@ -82,8 +94,33 @@ static void print_info(struct EtsHashTable *hash_table)
 }
 #endif
 
-EtsHashtableErrorCode ets_hashtable_insert(struct EtsHashTable *hash_table, term key, term entry, EtsHashtableOptions opts, Heap *heap, GlobalContext *global)
+struct HNode *ets_hashtable_new_node(term entry, int keypos)
 {
+
+    struct HNode *new_node = malloc(sizeof(struct HNode));
+    if (IS_NULL_PTR(new_node)) {
+        return NULL;
+    }
+
+    size_t size = (size_t) memory_estimate_usage(entry);
+    if (memory_init_heap(&new_node->heap, size) != MEMORY_GC_OK) {
+        free(new_node);
+        return NULL;
+    }
+
+    term new_entry = memory_copy_term_tree(&new_node->heap, entry);
+    term key = term_get_tuple_element(new_entry, keypos);
+
+    new_node->next = NULL;
+    new_node->key = key;
+    new_node->entry = new_entry;
+
+    return new_node;
+}
+
+EtsHashtableErrorCode ets_hashtable_insert(struct EtsHashTable *hash_table, struct HNode *new_node, EtsHashtableOptions opts, GlobalContext *global)
+{
+    term key = new_node->key;
     uint32_t hash = hash_term(key, global);
     uint32_t index = hash % hash_table->capacity;
 
@@ -94,38 +131,30 @@ EtsHashtableErrorCode ets_hashtable_insert(struct EtsHashTable *hash_table, term
 #endif
 
     struct HNode *node = hash_table->buckets[index];
-    if (node) {
-        while (1) {
-            if (term_compare(key, node->key, TermCompareExact, global) == TermEquals) {
-                if (opts & EtsHashtableAllowOverwrite) {
-                    node->entry = entry;
-                    memory_destroy_heap(node->heap, global);
-                    node->heap = heap;
-                    return EtsHashtableOk;
+    struct HNode *last_node = NULL;
+    while (node) {
+        if (term_compare(key, node->key, TermCompareExact, global) == TermEquals) {
+            if (opts & EtsHashtableAllowOverwrite) {
+                if (IS_NULL_PTR(last_node)) {
+                    new_node->next = node->next;
+                    hash_table->buckets[index] = new_node;
                 } else {
-                    return EtsHashtableFailure;
+                    last_node->next = new_node;
+                    new_node->next = node->next;
                 }
-            }
-
-            if (node->next) {
-                node = node->next;
+                ets_hashtable_free_node(node, global);
+                return EtsHashtableOk;
             } else {
-                break;
+                ets_hashtable_free_node(new_node, global);
+                return EtsHashtableFailure;
             }
         }
+        last_node = node;
+        node = node->next;
     }
 
-    struct HNode *new_node = malloc(sizeof(struct HNode));
-    if (IS_NULL_PTR(new_node)) {
-        return EtsHashtableError;
-    }
-    new_node->next = NULL;
-    new_node->key = key;
-    new_node->entry = entry;
-    new_node->heap = heap;
-
-    if (node) {
-        node->next = new_node;
+    if (last_node) {
+        last_node->next = new_node;
     } else {
         hash_table->buckets[index] = new_node;
     }
@@ -165,7 +194,7 @@ bool ets_hashtable_remove(struct EtsHashTable *hash_table, term key, size_t keyp
         term key_to_compare = term_get_tuple_element(node->entry, keypos);
         if (term_compare(key, key_to_compare, TermCompareExact, global) == TermEquals) {
 
-            memory_destroy_heap(node->heap, global);
+            memory_destroy_heap(&node->heap, global);
             struct HNode *next_node = node->next;
             free(node);
 
diff --git a/src/libAtomVM/ets_hashtable.h b/src/libAtomVM/ets_hashtable.h
@@ -52,9 +52,11 @@ typedef enum EtsHashtableErrorCode
 struct EtsHashTable *ets_hashtable_new();
 void ets_hashtable_destroy(struct EtsHashTable *hash_table, GlobalContext *global);
 
-EtsHashtableErrorCode ets_hashtable_insert(struct EtsHashTable *hash_table, term key, term entry, EtsHashtableOptions opts, Heap *heap, GlobalContext *global);
+EtsHashtableErrorCode ets_hashtable_insert(struct EtsHashTable *hash_table, struct HNode *new_node, EtsHashtableOptions opts, GlobalContext *global);
 term ets_hashtable_lookup(struct EtsHashTable *hash_table, term key, size_t keypos, GlobalContext *global);
 bool ets_hashtable_remove(struct EtsHashTable *hash_table, term key, size_t keypos, GlobalContext *global);
+struct HNode *ets_hashtable_new_node(term entry, int keypos);
+void ets_hashtable_free_node_array(struct HNode **allocated, size_t len, GlobalContext *global);
 
 #ifdef __cplusplus
 }
diff --git a/src/libAtomVM/nifs.c b/src/libAtomVM/nifs.c
@@ -3327,10 +3327,6 @@ static term nif_ets_insert(Context *ctx, int argc, term argv[])
     VALIDATE_VALUE(ref, is_ets_table_id);
 
     term entry = argv[1];
-    VALIDATE_VALUE(entry, term_is_tuple);
-    if (term_get_tuple_arity(entry) < 1) {
-        RAISE_ERROR(BADARG_ATOM);
-    }
 
     EtsErrorCode result = ets_insert(ref, entry, ctx);
     switch (result) {
diff --git a/tests/erlang_tests/test_ets.erl b/tests/erlang_tests/test_ets.erl
@@ -31,7 +31,7 @@ start() ->
     ok = test_protected_access(),
     ok = test_public_access(),
     ok = test_lookup_element(),
-
+    ok = test_insert_list(),
     0.
 
 test_basic() ->
@@ -352,3 +352,18 @@ test_lookup_element() ->
     expect_failure(fun() -> ets:lookup_element(Tid, foo, 3) end),
     expect_failure(fun() -> ets:lookup_element(Tid, foo, 0) end),
     ok.
+
+test_insert_list() ->
+    Tid = ets:new(test_insert_list, []),
+    true = ets:insert(Tid, [{foo, tapas}, {batat, batat}, {patat, patat}]),
+    true = ets:insert(Tid, [{foo, tapas}, {batat, batat}, {patat, patat}]),
+    [{patat, patat}] = ets:lookup(Tid, patat),
+    [{batat, batat}] = ets:lookup(Tid, batat),
+    true = ets:insert(Tid, []),
+    expect_failure(fun() -> ets:insert(Tid, [{foo, tapas} | {patat, patat}]) end),
+    expect_failure(fun() -> ets:insert(Tid, [{foo, tapas}, {batat, batat}, {patat, patat}, {}]) end),
+    expect_failure(fun() ->
+        ets:insert(Tid, [{foo, tapas}, pararara, {batat, batat}, {patat, patat}])
+    end),
+    expect_failure(fun() -> ets:insert(Tid, [{}]) end),
+    ok.
