Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+170 −68
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01456a3
Add option to insert list in ets:insert, ets:lookup refactor
TheSobkiewicz Jan 7, 2025
0f71f40
Insert list refactor. Moved node creation to separate function
TheSobkiewicz Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion libs/estdlib/src/ets.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down
121 changes: 88 additions & 33 deletions src/libAtomVM/ets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,58 +252,102 @@ 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);
return EtsBadEntry;
}

TheSobkiewicz marked this conversation as resolved.
Show resolved Hide resolved
Heap *heap = malloc(sizeof(Heap));
if (IS_NULL_PTR(heap)) {
SMP_UNLOCK(ets_table);
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;
}
size_t size = (size_t) memory_estimate_usage(entry);
if (memory_init_heap(heap, size) != MEMORY_GC_OK) {
free(heap);
SMP_UNLOCK(ets_table);
return EtsAllocationFailure;

EtsErrorCode result = EtsOk;
EtsHashtableErrorCode res = ets_hashtable_insert(ets_table->hashtable, new_node, EtsHashtableAllowOverwrite, ctx->global);
if (UNLIKELY(res != EtsHashtableOk)) {
result = EtsAllocationFailure;
}

term new_entry = memory_copy_term_tree(heap, entry);
term key = term_get_tuple_element(new_entry, (int) ets_table->keypos);
return result;
}

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;
}
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;
}

SMP_UNLOCK(ets_table);
struct HNode **nodes = malloc(size * sizeof(struct HNode *));
if (IS_NULL_PTR(nodes)) {
return EtsAllocationFailure;
}

return ret;
int i = 0;
while (term_is_nonempty_list(list)) {
TheSobkiewicz marked this conversation as resolved.
Show resolved Hide resolved
term tuple = term_get_list_head(list);
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;
}

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;
}
TheSobkiewicz marked this conversation as resolved.
Show resolved Hide resolved

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;
}

Expand All @@ -316,24 +360,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
}
Expand Down Expand Up @@ -368,9 +423,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;
}
Expand Down
89 changes: 61 additions & 28 deletions src/libAtomVM/ets_hashtable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand All @@ -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)
TheSobkiewicz marked this conversation as resolved.
Show resolved Hide resolved
{
Heap *heap = malloc(sizeof(Heap));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;

Expand All @@ -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;
}
Expand Down
4 changes: 3 additions & 1 deletion src/libAtomVM/ets_hashtable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
4 changes: 0 additions & 4 deletions src/libAtomVM/nifs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3323,10 +3323,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) {
Expand Down
17 changes: 16 additions & 1 deletion tests/erlang_tests/test_ets.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ start() ->
ok = test_protected_access(),
ok = test_public_access(),
ok = test_lookup_element(),

ok = test_insert_list(),
0.

test_basic() ->
Expand Down Expand Up @@ -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),
TheSobkiewicz marked this conversation as resolved.
Show resolved Hide resolved
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.
Loading