Insert list refactor. Moved node creation to separate function

Signed-off-by: Tomasz Sobkiewicz <[email protected]>
atomvm · Jan 22, 2025 · 0f71f40 · 0f71f40
1 parent 01456a3
commit 0f71f40
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added `net:gethostname/0` on platforms with gethostname(3).
 - Added `socket:getopt/2`
 - Added `supervisor:terminate_child/2`, `supervisor:restart_child/2` and `supervisor:delete_child/2`
+- Added support for list insertion in 'ets:insert/2'.
 
 ### Fixed
 - ESP32: improved sntp sync speed from a cold boot.
@@ -25,7 +26,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Added the ability to run beams from the CLI for Generic Unix platform (it was already possible with nodejs and emscripten).
 - Added support for 'erlang:--/2'.
-- Added support for list insertion in 'ets:insert/2'.
 
 ### Fixed
 

diff --git a/src/libAtomVM/ets.c b/src/libAtomVM/ets.c
@@ -262,21 +262,15 @@ static EtsErrorCode ets_table_insert(struct EtsTable *ets_table, term entry, Con
         return EtsBadEntry;
     }
 
-    Heap *heap = malloc(sizeof(Heap));
-    if (IS_NULL_PTR(heap)) {
-        return EtsAllocationFailure;
-    }
-    size_t size = (size_t) memory_estimate_usage(entry);
-    if (memory_init_heap(heap, size) != MEMORY_GC_OK) {
-        free(heap);
+    int keypos = (int) ets_table->keypos;
+
+    struct HNode *new_node = ets_hashtable_new_node(entry, keypos, ctx->global);
+    if (IS_NULL_PTR(new_node)) {
         return EtsAllocationFailure;
     }
 
-    term new_entry = memory_copy_term_tree(heap, entry);
-    term key = term_get_tuple_element(new_entry, (int) ets_table->keypos);
-
     EtsErrorCode result = EtsOk;
-    EtsHashtableErrorCode res = ets_hashtable_insert(ets_table->hashtable, key, new_entry, EtsHashtableAllowOverwrite, heap, ctx->global);
+    EtsHashtableErrorCode res = ets_hashtable_insert(ets_table->hashtable, new_node, EtsHashtableAllowOverwrite, ctx->global);
     if (UNLIKELY(res != EtsHashtableOk)) {
         result = EtsAllocationFailure;
     }
@@ -286,29 +280,47 @@ static EtsErrorCode ets_table_insert(struct EtsTable *ets_table, term entry, Con
 
 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;
+    }
     term iter = list;
+    size_t size = 0;
 
     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;
+    }
+
+    int i = 0;
     while (term_is_nonempty_list(list)) {
         term tuple = term_get_list_head(list);
-        EtsErrorCode result = ets_table_insert(ets_table, tuple, ctx);
-        if (UNLIKELY(result != EtsOk)) {
-            AVM_ABORT(); // Abort because operation might not be atomic.
+        nodes[i] = ets_hashtable_new_node(tuple, ets_table->keypos, ctx->global);
+        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) {
+        ets_hashtable_insert(ets_table->hashtable, nodes[i], EtsHashtableAllowOverwrite, ctx->global); // EtsHashtableAllowOverwrite cannot be changed here because it will result in data inconsistency.
+    }
+
+    free(nodes);
     return EtsOk;
 }
 

diff --git a/src/libAtomVM/ets_hashtable.c b/src/libAtomVM/ets_hashtable.c
@@ -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);
             struct HNode *next_node = node->next;
-            free(node);
+            ets_hashtable_free_node(node, global);
             node = next_node;
         }
     }
@@ -82,8 +94,37 @@ 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, GlobalContext *global)
+{
+    Heap *heap = malloc(sizeof(Heap));
+    if (IS_NULL_PTR(heap)) {
+        return NULL;
+    }
+    size_t size = (size_t) memory_estimate_usage(entry);
+    if (memory_init_heap(heap, size) != MEMORY_GC_OK) {
+        free(heap);
+        return NULL;
+    }
+
+    term new_entry = memory_copy_term_tree(heap, entry);
+    struct HNode *new_node = malloc(sizeof(struct HNode));
+    if (IS_NULL_PTR(new_node)) {
+        memory_destroy_heap(heap, global);
+        return NULL;
+    }
+    term key = term_get_tuple_element(new_entry, keypos);
+
+    new_node->next = NULL;
+    new_node->key = key;
+    new_node->entry = new_entry;
+    new_node->heap = heap;
+
+    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 +135,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;
     }

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, GlobalContext *global);
+void ets_hashtable_free_node_array(struct HNode **allocated, size_t len, GlobalContext *global);
 
 #ifdef __cplusplus
 }

diff --git a/tests/erlang_tests/test_ets.erl b/tests/erlang_tests/test_ets.erl
@@ -356,6 +356,7 @@ test_lookup_element() ->
 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, []),