Add option to insert list in ets:insert, ets:lookup refactor #1405

TheSobkiewicz · 2024-12-17T16:01:18Z

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

Changes:

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.

Use Cases for the Helper Functions:

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.

bettio · 2024-12-22T21:16:11Z

src/libAtomVM/ets.c

+        }
+        EtsErrorCode result = ets_table_insert(ets_table, tuple, ctx);
+        if (result != EtsOk) {
+            AVM_ABORT(); // Abort because operation might not be atomic.


We usually don't do VM abort: calling AVM_ABORT() means that an unrecoverable happened, such as memory corruption, a bad internal bug and any other kind of situation that required an entire VM crash and reboot.
Are we in this specific situation?

Now we don't have any other tool to ensure atomicity here. In case the insert fails at the Nth element, elements (0,N -1) will be inserted into the list, which could result in hard-to-debug behavior. It is unlikely to happen.

To expand: without this abort, if we're short on memory, we'd leave list partially inserted. If someone tries to persist inserts someday we'd leave the system in inconsistent state.

To avoid that we need to either abort or allocate the list of previous values and rollback in case of error (ensuring that nothing allocates in rollback path since we're most likely dealing with OOM). Abort is easier to do here.

This check needs to have UNLIKELY.

Ok, fair point, there is another feasible approach:
table nodes can be pre-allocated before making any change to the list, so in case of allocation failure freeing up allocated nodes can be easily done before making any actual change.

ets_hashtable_insert will need an additional node parameter, and a dedicated allocation function might be created (e.g. ets_hashtable_new_node). Furthermore key and and entry parameters can be moved to the ets_hashtable_new_node function if it can help.
This change will have a very small impact since ets_hashtable_insert is used in just one or two places.
I suggest doing this with an additional commit inside this PR, so we can make the review easier and separate this activity in 2 tasks.

This change will remove any implicit allocation and make abort not necessary.

src/libAtomVM/ets.c

tests/erlang_tests/test_ets.erl

src/libAtomVM/ets.c

jakub-gonet · 2025-01-12T01:44:52Z

src/libAtomVM/ets.c

+        return EtsTableNotFound;
+    }
+
+    EtsErrorCode result = ets_table_lookup(ets_table, key, ret, ctx);


While working on this code recently, I noticed that hashtable lookup take keypos arg which isn't needed (we have node->key and keypos can't change after table creation). May be worth to do it in this PR or in the followup.

Signed-off-by: Tomasz Sobkiewicz <[email protected]>

src/libAtomVM/ets.c

jakub-gonet · 2025-01-15T16:29:33Z

src/libAtomVM/ets_hashtable.c

+        return NULL;
+    }
+    size_t size = (size_t) memory_estimate_usage(entry);
+    if (memory_init_heap(heap, size) != MEMORY_GC_OK) {


I was wondering, why we create new heap instead of piggybacking on owner process' heap?

Short answer: this code is just a refactored existing code block that was doing the same.

Long answer: the GC is completely decoupled from ETS at the moment and ETS tables are not tied at all to a Context, so GC will not use their items as roots, so if we use the process heap we would screw up them.

If I understood correctly: they could be on the owner's heap if we could guarantee that they won't be GC'd.

right, that would require some bigger changes and there might be some extra complexity.

src/libAtomVM/ets_hashtable.c

bettio

Still few minor changes are required, thanks for this work so far.

bettio · 2025-01-18T07:03:05Z

CHANGELOG.md

@@ -27,6 +27,7 @@ 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 preliminary support for ESP32P4 (no networking support yet).
+- Added support for list insertion in 'ets:insert/2'.


I think it got by mistake in the wrong section of the changelog file.
Changed aimed to main branch should be under ## Unreleased (not in ## [0.6.6] - Unreleased section).
The right place should be just below Added supervisor:terminate_child/2, supervisor:restart_child/2`.

src/libAtomVM/ets_hashtable.c

bettio · 2025-01-18T07:21:29Z

src/libAtomVM/ets_hashtable.c

@@ -94,38 +131,34 @@ EtsHashtableErrorCode ets_hashtable_insert(struct EtsHashTable *hash_table, term
 #endif

    struct HNode *node = hash_table->buckets[index];
+    struct HNode *last_node = NULL;
    if (node) {


Not needed anymore since we are doing while (node)

Consider adding a node with completely new key. We're using linked lists for collisions. If you don't have last_node then node will be always null – you'd create a new node and then instead of attaching it to the end of a linked list, you'd add it directly to the bucket, overwriting existing nodes.

probably I didn't explain good enough, or maybe I'm overlooking something.
I mean that in the following snippet:

if (cond) { while (cond) { } }

if (cond) is redundant: if cond is false, while will be executed 0 times anyway.

Nope, you're absolutely right. Github didn't show enough context in inline diff to see that.

src/libAtomVM/ets_hashtable.h

src/libAtomVM/ets_hashtable.c

bettio · 2025-01-22T16:02:31Z

src/libAtomVM/ets_hashtable.c

 {
+    Heap *heap = malloc(sizeof(Heap));


Small trick and optimization:
Heap struct can be embedded inside HNode this way:

struct HNode { struct HNode *next; term key; term entry; Heap heap; };

Also, this will allow to avoid malloc overhead, simplifies code and also reduces memory fragmentation

bettio · 2025-01-22T16:11:26Z

src/libAtomVM/ets_hashtable.c

@@ -94,38 +131,34 @@ EtsHashtableErrorCode ets_hashtable_insert(struct EtsHashTable *hash_table, term
 #endif

    struct HNode *node = hash_table->buckets[index];
+    struct HNode *last_node = NULL;
    if (node) {


probably I didn't explain good enough, or maybe I'm overlooking something.
I mean that in the following snippet:

if (cond) { while (cond) { } }

if (cond) is redundant: if cond is false, while will be executed 0 times anyway.

bettio · 2025-01-22T16:15:31Z

src/libAtomVM/ets_hashtable.c

+        return NULL;
+    }
+    size_t size = (size_t) memory_estimate_usage(entry);
+    if (memory_init_heap(heap, size) != MEMORY_GC_OK) {


right, that would require some bigger changes and there might be some extra complexity.

src/libAtomVM/ets_hashtable.c

bettio · 2025-01-22T16:45:19Z

CHANGELOG.md

@@ -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'.


Pure nitpicking moment here, I always try to keep "changes local".
Let me explain better:
Let's suppose one day we find that the commit for pre-allocating nodes introduces a regression, the easiest solution would be just doing git revert 54951ffa8176cbec5ce757313abe60c9f38c8395, but this would revert back also the changelog fix, and the entry would jump into the wrong section.
The ideal approach is using git commit --fixup=01456a3e7544a144acbcf57e58b2adb02e270d50 and then git rebase --autosquash so the fixup is applied to the right commit, so changes belong to the right commit and reverting one doesn't revert unrelated changes.

bettio · 2025-01-22T16:48:11Z

src/libAtomVM/ets_hashtable.c

                    memory_destroy_heap(node->heap, global);
-                    node->heap = heap;
+                    free(node);


I still think that grouping together memory_destroy_heap(node->heap, global) and free(node) in a small helper function would make harder to forget one of the two.

Good idea, will refactor every occurrence of this.

bettio · 2025-01-22T16:49:49Z

I'm sorry about this additional round of comments, hope they can be somehow interesting to you and not just boring nitpicking.
Thanks for the contribution so far ❤️.

TheSobkiewicz · 2025-01-22T17:09:18Z

I'm sorry about this additional round of comments, hope they can be somehow interesting to you and not just boring nitpicking. Thanks for the contribution so far ❤️.

Don't worry, thank you for your patience when reviewing it ❤️

Signed-off-by: Tomasz Sobkiewicz <[email protected]>

This was referenced Dec 17, 2024

Add ets:update_counter TheSobkiewicz/AtomVM#1

Closed

Add ets:update_counter #1406

Open

bettio requested changes Dec 22, 2024

View reviewed changes

TheSobkiewicz mentioned this pull request Jan 7, 2025

Add ets:delete/1, ets:delete refactor #1461

Open

TheSobkiewicz force-pushed the thesobkiewicz/nifs/ets/refactor_insert branch 4 times, most recently from 76774f0 to 6ac7831 Compare January 9, 2025 15:44

jakub-gonet suggested changes Jan 12, 2025

View reviewed changes

TheSobkiewicz force-pushed the thesobkiewicz/nifs/ets/refactor_insert branch from 6ac7831 to c2bc9d2 Compare January 12, 2025 03:18

Add option to insert list in ets:insert, ets:lookup refactor

01456a3

Signed-off-by: Tomasz Sobkiewicz <[email protected]>

TheSobkiewicz force-pushed the thesobkiewicz/nifs/ets/refactor_insert branch 2 times, most recently from f54ef62 to 45eccdc Compare January 15, 2025 16:13

jakub-gonet suggested changes Jan 15, 2025

View reviewed changes

TheSobkiewicz force-pushed the thesobkiewicz/nifs/ets/refactor_insert branch from 45eccdc to e5908a2 Compare January 16, 2025 12:56

bettio requested changes Jan 18, 2025

View reviewed changes

TheSobkiewicz force-pushed the thesobkiewicz/nifs/ets/refactor_insert branch 3 times, most recently from 387dc1c to 54951ff Compare January 20, 2025 14:25

bettio requested changes Jan 22, 2025

View reviewed changes

TheSobkiewicz force-pushed the thesobkiewicz/nifs/ets/refactor_insert branch 2 times, most recently from 4bc9bb5 to 6e249eb Compare January 22, 2025 17:35

Insert list refactor. Moved node creation to separate function

0f71f40

Signed-off-by: Tomasz Sobkiewicz <[email protected]>

TheSobkiewicz force-pushed the thesobkiewicz/nifs/ets/refactor_insert branch from 6e249eb to 0f71f40 Compare January 22, 2025 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to insert list in ets:insert, ets:lookup refactor #1405

Add option to insert list in ets:insert, ets:lookup refactor #1405

TheSobkiewicz commented Dec 17, 2024

bettio Dec 22, 2024

TheSobkiewicz Jan 7, 2025 •

edited

Loading

jakub-gonet Jan 12, 2025 •

edited

Loading

bettio Jan 13, 2025

jakub-gonet Jan 12, 2025

jakub-gonet Jan 15, 2025

bettio Jan 18, 2025

jakub-gonet Jan 18, 2025

bettio Jan 22, 2025

bettio left a comment

bettio Jan 18, 2025

bettio Jan 18, 2025

jakub-gonet Jan 18, 2025

bettio Jan 22, 2025

jakub-gonet Jan 22, 2025

bettio Jan 22, 2025

bettio Jan 22, 2025

bettio Jan 22, 2025

bettio Jan 22, 2025

bettio Jan 22, 2025

TheSobkiewicz Jan 22, 2025 •

edited

Loading

bettio commented Jan 22, 2025 •

edited

Loading

TheSobkiewicz commented Jan 22, 2025

Add option to insert list in ets:insert, ets:lookup refactor #1405

Are you sure you want to change the base?

Add option to insert list in ets:insert, ets:lookup refactor #1405

Conversation

TheSobkiewicz commented Dec 17, 2024

Changes:

Use Cases for the Helper Functions:

Choose a reason for hiding this comment

TheSobkiewicz Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

jakub-gonet Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bettio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSobkiewicz Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

bettio commented Jan 22, 2025 • edited Loading

TheSobkiewicz commented Jan 22, 2025

TheSobkiewicz Jan 7, 2025 •

edited

Loading

jakub-gonet Jan 12, 2025 •

edited

Loading

TheSobkiewicz Jan 22, 2025 •

edited

Loading

bettio commented Jan 22, 2025 •

edited

Loading