Experimental JS API based on tracking values#2013
Experimental JS API based on tracking values#2013jgonet wants to merge 5 commits intoatomvm:mainfrom
Conversation
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
It's a field in emscripten platform struct. It could be a callback entirely in JS but we use threads. Threads are emulated via webworkers which have their own contexts and variables aren't shared with each other. This means that we would lose uniqueness of the keys. This commit also changes output format from file loaded and ran at <script src=...> to ES6 classes which have greater control over initialization of Wasm. Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
|
cc @pguyot |
bettio
left a comment
There was a problem hiding this comment.
I added just some minor comments.
I'm not the emscripten expert here, however it would be good (and useful) having some documentation in emscripten module (and where it makes sense) about how this is supposed to be used. Some additional documentation would be super appreciated during review.
| term result = term_invalid_term(); | ||
| term refs = term_nil(); | ||
| if (UNLIKELY(memory_ensure_free_opt(target_ctx, TUPLE_SIZE(2) + LIST_SIZE(keys_n, TERM_BOXED_REFC_BINARY_SIZE), MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { | ||
| // TODO: how to raise? |
There was a problem hiding this comment.
It is. I pretty much copied this code from Emscripten function running JS script async (nif_emscripten_run_script). As far, as I understand it, we set trap signal and trigger it by returning term_invalid_term() which makes process wait for the signal to arrive.
I'm not sure how to do the signal sending part and raise at the same time. Should I just set result = term_invalid_term and override regs?
| free(keys); | ||
| mailbox_send_term_signal(target_ctx, TrapAnswerSignal, result); | ||
| globalcontext_get_process_unlock(global, target_ctx); | ||
| } else { |
There was a problem hiding this comment.
style: you can either remove this } else { and just free() at the end of function, or even do an early return when target_ctx == NULL. Maybe the second option is more readable.
There was a problem hiding this comment.
I'm fine with either. Should we also change run_script to do it the same?
| }); | ||
| // clang-format on | ||
|
|
||
| static void do_get_tracked_objects(uint32_t *ref_keys, size_t keys_n, int32_t sync_caller_pid, GlobalContext *global) |
There was a problem hiding this comment.
Right now we are using uint32_t for process ids. Also we are using the extended process_id name instead of pid when talking about the id itself (not encoded as a term having type pid).
| }); | ||
| // clang-format on | ||
|
|
||
| static void do_run_script_tracked(const char *script, int32_t sync_caller_pid, GlobalContext *global) |
There was a problem hiding this comment.
Right now we are using uint32_t for process ids. Also we are using the extended process_id name instead of pid when talking about the id itself (not encoded as a term having type pid).
| target_link_options(AtomVM PRIVATE -sEXPORTED_RUNTIME_METHODS=ccall -sUSE_ZLIB=1 -O3 -pthread -sFETCH -lwebsocket.js --pre-js ${CMAKE_CURRENT_SOURCE_DIR}/atomvm.pre.js) | ||
|
|
||
| if (AVM_USE_WASM_MJS) | ||
| target_link_options(AtomVM PRIVATE -sEXPORTED_RUNTIME_METHODS=ccall,cwrap,stringToNewUTF8 -sEMULATE_FUNCTION_POINTER_CASTS=1 -sEXPORTED_FUNCTIONS=_malloc,_cast,_call,_next_tracked_object_key,_main -sEXPORT_ES6=1 -sUSE_ZLIB=1 -O3 -pthread -sFETCH -lwebsocket.js --pre-js ${CMAKE_CURRENT_SOURCE_DIR}/atomvm.pre.js) |
There was a problem hiding this comment.
-O3 if a pretty aggressive optimization option. Do we really want to hardcode it?
There was a problem hiding this comment.
Agreed. I vaguely recall that CMake has some standard compiler/linker env vars so we could use them. Either way, choice between -O3 and -Os would be nice.
I just retained current state which hardcodes -O3.
| { | ||
| static const uint8_t OK = 0; | ||
| static const uint8_t BAD_KEY = 1; | ||
| static const uint8_t NOT_STRING = 2; |
There was a problem hiding this comment.
Shouldn't we define an enum here?
There was a problem hiding this comment.
I think I did it that way out of reflex when writing JS code above. I'll change it.
| if (strcmp("run_script/2", nifname) == 0) { | ||
| return &emscripten_run_script_nif; | ||
| } | ||
| if (strcmp("run_script_tracked/1", nifname) == 0) { |
There was a problem hiding this comment.
They should be added to emscripten module: libs/eavmlib/src/emscripten.erl
typespecs and documentation about how they should be used is appreciated.
pguyot
left a comment
There was a problem hiding this comment.
I checked out the branch, but it doesn't compile.
For example:
src/platforms/emscripten/src/lib/platform_nifs.c:164:76: error: no member named 'tracked_object_resource_type' in 'struct EmscriptenPlatformData'
164 | struct TrackedObjectResource *rsrc_obj = enif_alloc_resource(platform->tracked_object_resource_type, sizeof(struct TrackedObjectResource));
I am not completely clear about what you want to achieve with tracking values even if I have some intuition that it should be good. Could you please explain it further? I would be more than happy to help you write tests for this platform. We have some cypress-based tests as well as some node-based tests.
Also I don't see this change (and I probably would want to challenge it):
We also need JS to have access to DOM. We hardcoded thread executing JS to be main thread (or in our case, iframe thread).
We were thinking about relaxing that constraint but it'd need js_tracked_eval to forward script to it's destination JS context (e.g. postMessage() to iframe).
The E6 change could be in a separate PR and I am not really aware of the pros and cons. It sounds like a good idea and I'm not sure we want an option.
| return term_invalid_term(); | ||
| } | ||
| rsrc_obj->key = key; | ||
| term obj = enif_make_resource(erl_nif_env_from_context(ctx), rsrc_obj); |
There was a problem hiding this comment.
We made enif_make_resource deprecated because it can abort. You should use memory_erl_nif_env_ensure_free followed by term_from_resource instead.
| if (result === null) { | ||
| return; | ||
| } | ||
| if (Array.isArray(result) && keys.every(isIndex)) { |
There was a problem hiding this comment.
I'm not sure that keys exists. You probably mean result.every(isIndex)
Was this tested at all?
| size_t sys_get_next_tracked_object_key(GlobalContext *glb) | ||
| { | ||
| struct EmscriptenPlatformData *platform = glb->platform_data; | ||
| return platform->next_tracked_object_key++; |
There was a problem hiding this comment.
next_tracked_object_key is an atomic_size_t but post-increment is not atomic in C11 AFAIK.
|
|
||
| if (type == KEY_ATOM) { | ||
| if (UNLIKELY(memory_ensure_free_opt(ctx, LIST_SIZE(n, 1), MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { | ||
| RAISE_ERROR(OUT_OF_MEMORY_ATOM); |
There was a problem hiding this comment.
ref_keys should be freed here.
| for (long i = n - 1; i >= 0; --i) { | ||
| keys = term_list_prepend(term_from_int32(ref_keys[i]), keys, &ctx->heap); | ||
| } | ||
| return keys; |
There was a problem hiding this comment.
ref_keys should be freed here.
| assert(type == VALUE_ATOM); | ||
| // Trap caller waiting for completion | ||
| context_update_flags(ctx, ~NoFlags, Trap); | ||
| emscripten_dispatch_to_thread(emscripten_main_runtime_thread_id(), EM_FUNC_SIG_VIIII, do_get_tracked_objects, NULL, ref_keys, n, ctx->process_id, ctx->global); |
There was a problem hiding this comment.
ref_keys is passed here, but do_get_tracked_objects doesn't free it.
| option(AVM_CREATE_STACKTRACES "Create stacktraces" ON) | ||
| option(AVM_BUILD_RUNTIME_ONLY "Only build the AtomVM runtime" OFF) | ||
| option(COVERAGE "Build for code coverage" OFF) | ||
| option(AVM_USE_WASM_MJS "Use ES modules for Emscripten platform" OFF) |
There was a problem hiding this comment.
Not sure about this option. Should this become the default? What are the pros and cons?
| target_link_options(AtomVM PRIVATE -sEXPORTED_RUNTIME_METHODS=ccall -sUSE_ZLIB=1 -O3 -pthread -sFETCH -lwebsocket.js --pre-js ${CMAKE_CURRENT_SOURCE_DIR}/atomvm.pre.js) | ||
|
|
||
| if (AVM_USE_WASM_MJS) | ||
| target_link_options(AtomVM PRIVATE -sEXPORTED_RUNTIME_METHODS=ccall,cwrap,stringToNewUTF8 -sEMULATE_FUNCTION_POINTER_CASTS=1 -sEXPORTED_FUNCTIONS=_malloc,_cast,_call,_next_tracked_object_key,_main -sEXPORT_ES6=1 -sUSE_ZLIB=1 -O3 -pthread -sFETCH -lwebsocket.js --pre-js ${CMAKE_CURRENT_SOURCE_DIR}/atomvm.pre.js) |
There was a problem hiding this comment.
Why do we need cwrap and stringToNewUTF8?
Why do we need EMULATE_FUNCTION_POINTER_CASTS=1 ?
|
|
||
| for (long i = keys_n - 1; i >= 0; --i) { | ||
| term tracked_object = term_tracked_object_from_key(target_ctx, keys[i]); | ||
| // we can't easily recover from OOM here |
There was a problem hiding this comment.
You return OUT_OF_MEMORY_ATOM above, why can't you do it here?
Our use-case for Popcorn is to allow sharing objects between Elixir and JS. When requesting a JS value from Elixir, we get a handle for value stored in JS context. Main motivation for this change was to allow garbage-collecting the underlying object in JS when VM drops the handle. I feel like the approach may be viable for C as well, where it can expose some handles to internal objects for VM to reuse (e.g. init and allow ADC to be accessed, deinit when VM loses the handle). I think of it as building on resource infra but maybe the overlap is big enough that abstraction on top is too much. I don't have experience with uC side of AtomVM.
That's a mistake on my part – the extracted changes are Popcorn's changes without Popcorn specific code. I thought I tested compilation at least, sorry (definitely didn't test runtime since creating examples takes time and this was more like a conversation starter than PR that I expected to merge right away). My main goal with submitting the PR was to get overview is the approach is ok or should we rewrite it in a different way. Given that it's five months old and I've lost all context for the code, I'm inclined to just close it – I don't have time to work on it in the near future.
I remember some suggestion to add it as option but I agree this should just be es6 module always. All
For cwrap – wrapping call, cast, and other API functions has less overhead than wrapping on demand for single calls every time. |
This PR upstreams API we're using in Popcorn.
It doesn't have tests or extended docs. I'd gladly take some instructions about testing Wasm. Docs will be added after first round of review – I need to know if approach is sane at all.
Design notes
We needed some API to track values in JS with their lifetime in the VM. This was achieved by using new type of resource –
TrackedValue. Popcorn also wants to customize JS behavior without maintaining custom Emscripten hooks in.cfiles – we extracted them to functions inatomvm.pre.js. You can see our implementation in Popcorn repo.We also need JS to have access to DOM. We hardcoded thread executing JS to be main thread (or in our case, iframe thread).
We were thinking about relaxing that constraint but it'd need
js_tracked_evalto forward script to it's destination JS context (e.g.postMessage()to iframe).js_get_tracked_objectsworks by batchingTrackedObjects and their status (missing, bad type, etc). This allows to reduce communication overhead between JS and Wasm.We also needed to use ES modules instead of current, IIFE format. It makes it easier to load in iframes and to use in modern JS projects.
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