Skip to content

Optimize allocation of memory in ESP32 socket_driver do_send #1526

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

Open
wants to merge 1 commit into
base: release-0.6
Choose a base branch
from
Open

Optimize allocation of memory in ESP32 socket_driver do_send #1526

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 49 additions & 28 deletions src/platforms/esp32/components/avm_builtins/socket_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,40 +1117,61 @@ static void do_send(Context *ctx, const GenMessage *gen_message)
term data = term_get_tuple_element(gen_message->req, 1);

size_t buffer_size;
switch (interop_iolist_size(data, &buffer_size)) {
case InteropOk:
break;
case InteropMemoryAllocFail:
fprintf(stderr, "error: failed alloc.\n");
return;
case InteropBadArg:
fprintf(stderr, "error: invalid iolist.\n");
return;
}
void *buffer = malloc(buffer_size);
switch (interop_write_iolist(data, buffer)) {
case InteropOk:
break;
case InteropMemoryAllocFail:
free(buffer);
fprintf(stderr, "error: failed alloc.\n");
return;
case InteropBadArg:
free(buffer);
fprintf(stderr, "error: invalid iolist.\n");
return;
const void *buffer;
void *buffer_copy = NULL;
if (term_is_binary(data)) {
// No need to copy.
buffer_size = term_binary_size(data);
buffer = term_binary_data(data);
} else {
switch (interop_iolist_size(data, &buffer_size)) {
case InteropOk:
break;
case InteropMemoryAllocFail:
if (UNLIKELY(memory_ensure_free(ctx, REPLY_SIZE) != MEMORY_GC_OK)) {
AVM_ABORT();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we send an out_of_memory AtomVM rather than abort here? Maybe the process should be allowed to crash instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for raising this. I copied what exists elsewhere, when we don't have enough RAM to send an out of memory message. I wonder what we could do. generic_unix also aborts but preallocates memory for the reply.

https://github.com/atomvm/AtomVM/blob/7396812e4bd379237644e55ad822f7798d79f9d8/src/platforms/generic_unix/lib/socket_driver.c#L1124C5-L1124C26

Alternatively, we could promote using the C stack for ports instead of the context heap which relies on malloc and GC.

Should we take this in another PR, though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Older code definitely aborts, but in updated drivers etc, when there isn't enough memory for a proper error return we simply return the out_of_memory atom alone. I think this is far better (since it will likely cause the process to crash) than aborting the whole VM, we should just go ahead and let the process crash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we could promote using the C stack for ports instead of the context heap which relies on malloc and GC.

This seems like it would be more likely for failed allocations to result in a panic, again crashing the entire VM, instead of just the users process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we take this in another PR, though?

This might be the best solution for now. Perhaps we need a meta-issue about reducing the use of abort in general in favor of returning an error and crashing user processes. There are quite a few places in the code base where we abort when returning an error would be preferable, like this one. It seems much better to return an error, that quit all together, when a payload fails to be sent. This might just be a transient low memory condition that occurs very infrequently during an applications execution, and might be remedied by supervisors that the user has already put in place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also just keep the original behavior of logging the failure to stderr and returning, in the hopes the next GC might free enough memory for normal operation to resume.

}
do_send_reply(ctx, OUT_OF_MEMORY_ATOM, ref_ticks, pid);
return;
case InteropBadArg:
if (UNLIKELY(memory_ensure_free(ctx, REPLY_SIZE) != MEMORY_GC_OK)) {
AVM_ABORT();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about aborting here... and again below several more times.

}
do_send_reply(ctx, BADARG_ATOM, ref_ticks, pid);
return;
}
buffer_copy = malloc(buffer_size);
switch (interop_write_iolist(data, buffer_copy)) {
case InteropOk:
break;
case InteropMemoryAllocFail:
free(buffer_copy);
if (UNLIKELY(memory_ensure_free(ctx, REPLY_SIZE) != MEMORY_GC_OK)) {
AVM_ABORT();
}
do_send_reply(ctx, OUT_OF_MEMORY_ATOM, ref_ticks, pid);
return;
case InteropBadArg:
free(buffer_copy);
if (UNLIKELY(memory_ensure_free(ctx, REPLY_SIZE) != MEMORY_GC_OK)) {
AVM_ABORT();
}
do_send_reply(ctx, BADARG_ATOM, ref_ticks, pid);
return;
}
buffer = buffer_copy;
}
err_t status = netconn_write(tcp_data->socket_data.conn, buffer, buffer_size, NETCONN_COPY);
free(buffer);
if (UNLIKELY(status != ERR_OK)) {
fprintf(stderr, "write error: %i\n", status);
return;
}
free(buffer_copy);

if (UNLIKELY(memory_ensure_free(ctx, REPLY_SIZE) != MEMORY_GC_OK)) {
AVM_ABORT();
}
do_send_reply(ctx, OK_ATOM, ref_ticks, pid);
if (UNLIKELY(status != ERR_OK)) {
do_send_reply(ctx, ERROR_ATOM, ref_ticks, pid);
} else {
do_send_reply(ctx, OK_ATOM, ref_ticks, pid);
}
}

static void do_sendto(Context *ctx, const GenMessage *gen_message)
Expand Down
Loading