Optimize allocation of memory in ESP32 socket_driver do_send #1526
Conversation
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
| break; | ||
| case InteropMemoryAllocFail: | ||
| if (UNLIKELY(memory_ensure_free(ctx, REPLY_SIZE) != MEMORY_GC_OK)) { | ||
| AVM_ABORT(); |
There was a problem hiding this comment.
Should we send an out_of_memory AtomVM rather than abort here? Maybe the process should be allowed to crash instead?
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| return; | ||
| case InteropBadArg: | ||
| if (UNLIKELY(memory_ensure_free(ctx, REPLY_SIZE) != MEMORY_GC_OK)) { | ||
| AVM_ABORT(); |
There was a problem hiding this comment.
Same question about aborting here... and again below several more times.
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