Is the JobQueue Port API a good abstraction?

[akosthekiss]

TL;DR: Is there any other way to implement a Promise job queue than what's implemented in the default port? If not, should it not be merged into jerry-core?

Source of my confusion:

• If Promises are available/enabled then those JS functions that are in thens or catches are to be executed after the main script completed. Until they get their turn, they are enqueued by jerry_port_jobqueue_enqueue.
• Enqueued jobs must be executed, otherwise they keep references to their environment and memory will not be freed. (Just try to comment out jerry_port_default_jobqueue_run in main-unix.c, the debug-built jerry tool will assert out at cleanup when running a script with Promises, because heap does not get emptied.)
• However, there is no general API to execute the enqueued jobs. The default port provides jerry_port_default_jobqueue_run, but it is specific to that port.
• What's more, the default port implementation uses the engine's internal memory handling routines (jmem_heap_alloc_block, jmem_heap_free_block), which is quite a no-no IMHO.

I've taken a quick look at another VM (JavaScriptCore) and command line tool (jsc), how it handles Promises and enqueued jobs. Without digging too deep, it seems to me that there the VM has an API call to execute such jobs (called microtasks, if I'm not mistaken).
(vm.drainMicrotasks(); at https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/jsc.cpp#L3522)

Wouldn't it be better for us, too, to integrate Promise job handling into the engine. E.g., something like

• jerry_run_enqueued to run one enqueued job,
• jerry_run_all_enqueued to run until the queue gets empty (replacement for jerry_port_default_jobqueue_run and analogous to JSC's vm.drainMicrotasks, I think).

Advantages:

• the whole API needed to run a Promise-enabled engine is port-independent (in addition to the above discussed, jerry_init can do the queue initialization instead of jerry_port_default_jobqueue_init),
• no API layering violation,
• less burden on port implementers.

However, I'm not sure whether there are any use cases for which this approach would be inappropriate.

Feedbacks are welcome.

[akosthekiss]

cc @jiangzidong @martijnthe

[martijnthe]

Prior discussion for the reasoning behind putting the queue implementation in the port:
#1685 (comment)

In the past projects I've worked on, there is already an event queue + loop in the project and JerryScript is added "on top". The path we took with the current queue port was chosen because it makes it easy to integrate it into such an existing set up.

In completely stripped down form, the event loop usually looks something like (pseudocode):

Event *e;
while (get_next_event_or_block(&e)) {
  // project specific stuff that needs to be done before handling an event
  e->handler(e);
  // project specific stuff that needs to be done after handling an event
};

get_next_event_or_block() will block if there are no events to process, until another task/thread that creates events puts a new event in the queue. So, the concern of sleeping the application task and receiving events from the queue are combined into one call.

Other designs are possible too. For example, in Zephyr.js these 2 concerns are separated.
See separate zjs_loop_block(): https://github.com/01org/zephyr.js/blob/master/src/main.c#L200

Maybe you could have:

1. void jerry_port_signal_job_enqueued(void): called by JrS so event loop can be unblocked
2. void jerry_run_all_enqueued(void): called from project inside event loop

There's probably a problem with 2: how to know when to call it? Maybe there are other events from the project itself in the event queue that need to be serviced before handling the promise callbacks?

FYI, Zephyr.js WIP port that implements the queue port API:
https://github.com/01org/zephyr.js/pull/1058/files#diff-98710712f714db21afb280b026219dc8

BTW, I'm not 100% sure if it's any different in the JSC code.
The function at the link you posted seems to be part of a REPL runner.

Maybe @jprestwo has some ideas on this too, since he is working on that Zephyr.js PR.

[akosthekiss]

@martijnthe Thanks for the pointer. Yes, that's a valid need / use case.

What about:

• void jerry_port_job_enqueued (void) - signaling the port that one job just got enqueued
• jerry_value_t jerry_run_enqueued (void) - executing one enqueued job (if there is any)
• jerry_value_t jerry_run_all_enqueued (void) - executing jobs one-by-one until the queue is empty (or until an error occurs? - but I'm not sure about that)

Then, the default port could have an empty implementation for jerry_port_job_enqueued and run jerry_run_all_enqueued at the end of the REPL.

And ZJS could have, e.g.,

static void job_callback(void *h, const void *args)
{
    (void)(args);
    (void)(h);
    jerry_run_enqueued();
    zjs_remove_callback(wrapper->id); // ???
}

void jerry_port_job_enqueued()
{
    zjs_callback_id id = zjs_add_c_callback(NULL, job_callback);
    zjs_signal_callback(id, NULL, 0);
}

(Copied over and adapted some code from the referenced commit. Not sure what to do with the ??? line, didn't dig into the API that deep.)

I think this solves the "how to know when to call it" question. At least it seems to me that it does exactly the same as the current variant.

[akosthekiss]

@martijnthe see a proof-of-concept commit: akosthekiss@add7460

[martijnthe]

Ah, I see, so you'd build on top of the mechanism to schedule a callback. Good idea.

When would you use jerry_run_all_enqueued() vs jerry_run_enqueued ()?

I wonder if it makes sense to add an opaque job pointer argument to jerry_port_job_enqueued() and jerry_run_enqueued() so there's a clear, 1:1 relationship between enqueuing and running a job.

I'm a bit puzzled why me and @jiangzidong didn't see / go for this solution instead.

cc @jprestwo thoughts?

[akosthekiss]

When would you use jerry_run_all_enqueued() vs jerry_run_enqueued ()?

jerry_run_all_enqueued is for the simplest of cases only, it's just a convenience function. For most real-life use cases, you'll need jerry_run_enqueued, I guess.

I'll open a PR for this, then. You can still downvote me there :)

[glistening]

@akosthekiss I found this issue just now. I have a plan to port promise on tizenrt after minimal porting ended. I'll check how much code is reusable in default port and give a proposal if necessary.

[martijnthe]

Can be closed now?

[akosthekiss]

Yes. #1804 has landed, closing the issue.