Support ECMAScript stopping in JerryScript. #1753

zherczeg · 2017-04-20T11:06:03Z

No description provided.

robertsipka · 2017-04-20T11:20:45Z

tools/build.py

@@ -109,6 +109,8 @@ def devhelp(helpstring):
                        help='build unittests')
    parser.add_argument('-v', '--verbose', action='store_const', const='ON', default='OFF',
                        help='increase verbosity')
+    parser.add_argument('--vm-exec-stop', metavar='X', choices=['ON', 'OFF'], default='ON', type=str.upper,


I think you also need to disable the vm execution stopping by default in here.

Good catch. Sorry.

martijnthe · 2017-04-20T11:25:01Z

docs/02.API-REFERENCE.md

+JerryScript executes an ECMASCript program.
+
+If the callback returns with undefined value the ECMAScript execution
+continues. Otherwise the result is converted to an error and thrown


Doesn't it make more sense to let the callback return an Error as opposed to letting the engine convert it into an error?

That way the callback can decide what Error (sub)type to use for this.

This would make the return value processing a bit more difficult since we would need to free the returned value, but would make the code more robust. If this is an acceptable overhead, I can change it.

Not sure if I follow.. If the return value is a Jerry string you don't need to free it while if it's an object (Error instance) you do?

It is not error case, but the normal case. If we return with undefined, we don't need to do anything (it is a primitive value). But if we allow to return with anything in the normal case, we need to prepare for complex objects, and free them.

Alternative suggestion. Return with:

undefined (with or without error flag): do nothing

anything else: if the value does not have an error flag, automatically add it. Throw the value.

Sounds good to me!

jiangzidong · 2017-04-21T02:38:31Z

tests/unit/test-api.c

+
+    int countdown = 6;
+
+    jerry_set_vm_exec_stop_callback (vm_exec_stop_callback, &countdown, 16);


a question to verify:
if the JS code do contains useful loop, e.g.

for (i = 0; i<6*16; i++) { a[i] = process(i); }

or multiple loops but the total VM_OC_BACKWARD_BRANCH bytecode exceeds 6*16, the case will also be stopped here, right?

I don't really understand this question. In your case the loop will probably stop at 6*16-1 index (a function start check and 6*16-1 backward loop checks).

To reduce the overhead the check only happens at backward loops and stack rollbacks (includes function entries).

Sorry, it's a bad example.
I wanted to ask whether the current design only count the VM_OC_BACKWARD_BRANCH, and then if countdown to 0, the user provided stop_cb will be invoked. And now I know the answer is YES.

To be more precise it counts the backward branches, function entries and stack rollbacks, but in your case there is only a single function entry (the global code is a function).

This code stops sooner:

for (i = 0; i<6*16; i++) { try { throw 1 } catch(e) { } }

Before you ask: why it counts stack rollbacks? Because we throw an exception which cause a stack rollback and an active catch block catches this exception and restart executing the byte code. However, our termination check runs again just before the code is executed, and raise another exception. At some point the stack will fully rolled back and the actual function returns with the exception. Then stack roll back start for the callee function.

Hm, function entry checks might not be needed. This could further optimize this code. But I need to think over this.

Thanks for your explanation :)

glistening · 2017-04-21T04:13:22Z

docs/02.API-REFERENCE.md

+static jerry_value_t
+vm_exec_stop_callback (void *user_p)
+{
+  static countdown = 10;


Probably, static int countdown = 10;

Yes! Thanks for catching it.

zherczeg · 2017-04-24T09:27:51Z

Patch updated, all comments addressed.

LaszloLango · 2017-04-24T09:29:41Z

Please rebase to the current master.

zherczeg · 2017-04-24T09:42:46Z

Rebased.

akosthekiss · 2017-04-26T10:46:50Z

docs/02.API-REFERENCE.md

@@ -35,7 +35,8 @@ Possible compile time enabled feature types:
 - JERRY_FEATURE_REGEXP_DUMP - regexp byte-code dumps
 - JERRY_FEATURE_SNAPSHOT_SAVE - saving snapshot files
 - JERRY_FEATURE_SNAPSHOT_EXEC - executing snapshot files
- - JERRY_FEATURE_DEBUGGER - debugging
+ - JERRY_FEATURE_DEBUGGER - debugger server is available


Why this change? It is both unrelated and makes the typedef out of sync from the doc.

akosthekiss · 2017-04-26T10:48:08Z

jerry-core/jerry.c

@@ -2149,6 +2152,31 @@ jerry_is_valid_cesu8_string (const jerry_char_t *cesu8_buf_p, /**< CESU-8 string
 } /* jerry_is_valid_cesu8_string */

 /**
+ * If JERRY_VM_EXEC_STOP is defined the periodically called callback


incomplete sentence?

akosthekiss · 2017-04-26T10:48:49Z

jerry-core/jerry.c

+void
+jerry_set_vm_exec_stop_callback (jerry_vm_exec_stop_callback_t stop_cb, /**< periodically called user function */
+                                 void *user_p, /**< pointer passed to the function */
+                                 uint32_t frequency) /**< frequency of the function call */


what is the unit of frequency?

This is described in the documentation. There is no real unit. If the callback could be called N times, it is actually called floor(N/frequency) times

akosthekiss · 2017-04-26T10:49:52Z

jerry-core/jerryscript.h

@@ -86,6 +86,7 @@ typedef enum
  JERRY_FEATURE_SNAPSHOT_SAVE, /**< saving snapshot files */
  JERRY_FEATURE_SNAPSHOT_EXEC, /**< executing snapshot files */
  JERRY_FEATURE_DEBUGGER, /**< debugging */
+  JERRY_FEATURE_VM_EXEC_STOP, /**< vm execution stopping enabled */


this is also not the same text as the one found in API docs

LaszloLango · 2017-04-26T10:53:29Z

docs/02.API-REFERENCE.md

+  jerry_value_t src = jerry_parse ((jerry_char_t *) src_p, strlen (src_p), false);
+  jerry_release_value (jerry_run (src));
+  jerry_release_value (src);
+}


missing jerry_cleanup ();

LaszloLango · 2017-04-26T11:00:32Z

docs/02.API-REFERENCE.md

@@ -3896,3 +3897,76 @@ jerry_parse_and_save_literals (const jerry_char_t *source_p,
 - [jerry_init](#jerry_init)
 - [jerry_cleanup](#jerry_cleanup)
 - [jerry_register_magic_strings](#jerry_register_magic_strings)
+
+


Based on jerryscript.h: add a new top level header here: # Miscellaneous functions.

LaszloLango · 2017-04-26T11:07:45Z

jerry-core/vm/vm.c

+              JERRY_CONTEXT (vm_exec_stop_counter) = 1;
+
+              left_value = ecma_make_simple_value (ECMA_SIMPLE_VALUE_UNDEFINED);
+              right_value = ecma_make_simple_value (ECMA_SIMPLE_VALUE_UNDEFINED);


left_value and right_value shouldn't be freed here?

It is freed after the error: label.

Sorry I wanted to write that the free happens after error: label above, and when we reach this position, the variables hold garbage. Since we go back to error: we would encounter a double free issue.

zherczeg · 2017-04-27T07:49:41Z

Patch updated, please check.

LaszloLango

LGTM

akosthekiss · 2017-04-27T11:26:55Z

docs/02.API-REFERENCE.md

+
+When JERRY_FEATURE_VM_EXEC_STOP is enabled a callback function can be
+specified by this function. This callback is periodically called when
+JerryScript executes an ECMASCript program.


ECMAScript (note the 'c')

akosthekiss · 2017-04-27T11:28:23Z

docs/02.API-REFERENCE.md

+If the callback returns with undefined value the ECMAScript execution
+continues. Otherwise the result is thrown by the engine (if the error
+flag is not set for the returned value the engine automatically sets
+it). The callback function might be called again even if it thrown


akosthekiss · 2017-04-27T11:32:40Z

docs/02.API-REFERENCE.md

+Frequently calling a callback in the main executor loop reduces
+its performance. The `frequency` argument allows decreasing
+this overhead. If its value is `N`, the callback is called only
+every `N` times. The minimum value of `N` is `1` (`0` is converted


I know I've asked that before but it's still not clear to me, at least from the doc, what this exactly means. "every N times" of what? Only after every N cycles of the VM, i.e., after the execution of every N byte code? Or after the execution of every N JS statement? Or something else? (I should not have to read the code to understand it, IMHO)

Please recommend me a good sentence then:

This is an extra layer for improving performance. Every time when the callback could be called, we decrease a counter, and only call the callback if it is 0. We also reset it back to frequency (that is why frequnce must be > 0, to avoid underflow). The dividers of CPU clock works this way. Basically if you set to to N then N-1 times the call is skipped, and only the Nth time it is executed.

I completely understand what N is for. What's undocumented is what "Every time when the callback could be called" means. When could the callback be called? After/before every byte code? After/before every JS statement?

akosthekiss · 2017-04-27T11:33:53Z

docs/02.API-REFERENCE.md

+  while (countdown > 0)
+  {
+    countdown--;
+    return jerry_create_undefined();


add space before ( (our examples should also follow our style guide)

akosthekiss · 2017-04-27T11:34:06Z

docs/02.API-REFERENCE.md

+  }
+
+  // The error flag is added automatically.
+  return jerry_create_string((const jerry_char_t *) "Abort script");


add space before (

akosthekiss · 2017-04-27T11:38:16Z

docs/02.API-REFERENCE.md

+
+```c
+void
+jerry_set_vm_exec_stop_callback (jerry_vm_exec_stop_callback_t stop_cb,


Shouldn't we document the signature of jerry_vm_exec_stop_callback_t, too? Right now, it can only be deduced from the example.

akosthekiss · 2017-04-27T11:47:19Z

jerry-core/vm/vm.c

+#ifdef JERRY_VM_EXEC_STOP
+          if (JERRY_CONTEXT (vm_exec_stop_cb) != NULL)
+          {
+            if (--JERRY_CONTEXT (vm_exec_stop_counter) == 0)


merge the two ifs?

if (JERRY_CONTEXT (vm_exec_stop_cb) != NULL && --JERRY_CONTEXT (vm_exec_stop_counter) == 0)

I don't know how -- works in that case. The code is clearer this way.

It is completely well-defined and standard-specified, there is no vagueness involved. && in C has short circuit evaluation. Please see ISO/IEC 9899 standard, section 6.5.13, "Logical AND operator" (http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf, http://www.open-std.org/JTC1/SC22/WG14/www/standards.html#9899)

I had enough problems with -- before. It is mostly undefined.

I would like the current code.

I'm really tempted to start a potentially long-running debate on "It is mostly undefined". (It is certainly not.) But I wont. Keep this as is.

akosthekiss · 2017-04-27T11:47:36Z

jerry-core/vm/vm.c

+#ifdef JERRY_VM_EXEC_STOP
+        if (JERRY_CONTEXT (vm_exec_stop_cb) != NULL)
+        {
+          if (--JERRY_CONTEXT (vm_exec_stop_counter) == 0)


likewise, merge the two ifs?

akosthekiss · 2017-04-27T11:48:51Z

tools/build.py

@@ -108,6 +108,8 @@ def devhelp(helpstring):
                        help='build unittests')
    parser.add_argument('-v', '--verbose', action='store_const', const='ON', default='OFF',
                        help='increase verbosity')
+    parser.add_argument('--vm-exec-stop', metavar='X', choices=['ON', 'OFF'], default='OFF', type=str.upper,
+                        help='enable vm execution stopping (%(choices)s; default: %(default)s)')


(nitpicking) VM?

akosthekiss · 2017-04-27T11:56:41Z

jerry-core/ecma/base/ecma-globals.h

+ * Callback which returns with a non-undefined value
+ * when the ECMAScript execution should be stopped.
+ */
+typedef ecma_value_t (*jerry_vm_exec_stop_cb_t) (void *user_p);


I've tried to figure out why this typedef duplication is needed (i.e., jerryscript.h also has jerry_vm_exec_stop_callback_t) but didn't succeed. We have other callback types in jerryscript.h, too (e.g., jerry_external_handler_t), but they have no duplicates internally. Couldn't we drop this also and use the single public typedef?

No. jerryscript.h is not included inside jerry-core. I don't want to use external_pointer_t, because thet requires lot of type casting. This is easier to understand.

This is clearly not true. jerry-core/jrt/jrt.h is including jerryscript.h (line 22) and jerry-core/ecma/base/ecma-globals.h is including jrt.h (line 20).

This is very bad news, I am not aware of that. I don't think removing this dependency should be in this patch though.

This is not bad news, this is how existing callback types are already used.

jerry_external_handler_t in ecma-function-object.c

jerry_object_free_callback_t in ecma-gc.c

jerry_user_context_deinit_cb in jcontext.h

They are all defined in jerryscript.h and used in the engine. There is no "dependency to be removed". It's the callback typedef duplication that's to be removed from ecma-globals.h.

I'm strongly insisting on dropping this internal typedef.

akosthekiss · 2017-04-27T17:15:59Z

jerry-core/vm/vm.c

+#ifdef JERRY_VM_EXEC_STOP
+          if (JERRY_CONTEXT (vm_exec_stop_cb) != NULL)
+          {
+            if (--JERRY_CONTEXT (vm_exec_stop_counter) == 0)


I'm really tempted to start a potentially long-running debate on "It is mostly undefined". (It is certainly not.) But I wont. Keep this as is.

akosthekiss · 2017-04-27T17:30:11Z

jerry-core/ecma/base/ecma-globals.h

+ * Callback which returns with a non-undefined value
+ * when the ECMAScript execution should be stopped.
+ */
+typedef ecma_value_t (*jerry_vm_exec_stop_cb_t) (void *user_p);


I'm strongly insisting on dropping this internal typedef.

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]

zherczeg · 2017-04-28T07:56:37Z

Follow-up: #1791

akosthekiss

LGTM

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]

robertsipka reviewed Apr 20, 2017

View reviewed changes

martijnthe reviewed Apr 20, 2017

View reviewed changes

zherczeg mentioned this pull request Apr 20, 2017

Interruptible VM loop via callback function #1752

Closed

jiangzidong reviewed Apr 21, 2017

View reviewed changes

glistening reviewed Apr 21, 2017

View reviewed changes

zherczeg force-pushed the stop_js_exec branch from ccb120d to 8dbafcd Compare April 24, 2017 09:25

zherczeg force-pushed the stop_js_exec branch 2 times, most recently from f9cf94a to 3f9b94e Compare April 24, 2017 09:42

zherczeg force-pushed the stop_js_exec branch 2 times, most recently from 77be87f to 270791f Compare April 25, 2017 12:29

akosthekiss requested changes Apr 26, 2017

View reviewed changes

LaszloLango requested changes Apr 26, 2017

View reviewed changes

LaszloLango reviewed Apr 26, 2017

View reviewed changes

zherczeg force-pushed the stop_js_exec branch from 270791f to 4f8ca05 Compare April 27, 2017 07:49

LaszloLango approved these changes Apr 27, 2017

View reviewed changes

akosthekiss requested changes Apr 27, 2017

View reviewed changes

zherczeg force-pushed the stop_js_exec branch from 4f8ca05 to ad7b4e2 Compare April 27, 2017 13:00

akosthekiss requested changes Apr 27, 2017

View reviewed changes

zherczeg force-pushed the stop_js_exec branch from ad7b4e2 to cf07c50 Compare April 28, 2017 07:40

Support ECMAScript stopping in JerryScript.

c04844c

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]

zherczeg force-pushed the stop_js_exec branch from cf07c50 to c04844c Compare April 28, 2017 07:43

akosthekiss approved these changes Apr 28, 2017

View reviewed changes

zherczeg merged commit 894aa6d into jerryscript-project:master Apr 28, 2017

zherczeg deleted the stop_js_exec branch April 28, 2017 12:19

jiangzidong pushed a commit to jiangzidong/jerryscript that referenced this pull request May 2, 2017

Support ECMAScript stopping in JerryScript. (jerryscript-project#1753)

4a61273

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]

zherczeg mentioned this pull request Mar 27, 2018

Nominating Robert Sipka (robertsipka) to become a reviewer #2254

Closed


		int countdown = 6;

		jerry_set_vm_exec_stop_callback (vm_exec_stop_callback, &countdown, 16);

Support ECMAScript stopping in JerryScript. #1753

Support ECMAScript stopping in JerryScript. #1753

Uh oh!

Conversation

zherczeg commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangzidong Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Apr 24, 2017

Uh oh!

LaszloLango commented Apr 24, 2017

Uh oh!

zherczeg commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Apr 27, 2017

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jiangzidong Apr 21, 2017 •

edited

Loading

zherczeg Apr 21, 2017 •

edited

Loading

zherczeg Apr 21, 2017 •

edited

Loading