module extension: add support for canonical name resolution #2013

gabrielschulhof · 2017-09-16T23:34:13Z

Before attempting to load a module, each provided resolver must be given an
opportunity to examine the name of the requested module without actually
loading it so as to canonicalize it, in case a module can be referred to by
multiple names.

Then, modules are loaded and cached by their canonical name.

LaszloLango · 2017-09-18T10:26:24Z

docs/12.EXT-REFERENCE-MODULE.md

+  jerry_value_t (*get_canonical_name) (const jerry_value_t name);
+  bool (*resolve) (const jerry_value_t canonical_name,
+                   jerry_value_t *result);
+};


} jerryx_module_resolver_t;

gabrielschulhof · 2017-09-18T10:32:06Z

@LaszloLango fixed.

zherczeg

Good patch overall.

zherczeg · 2017-09-18T10:51:30Z

docs/12.EXT-REFERENCE-MODULE.md

+{
+  jerry_value_t resulting_path;
+
+  /* Search name for "./" and "../", follow symlinks, etc., then create resulting_path via jerry_create_string () */


Full stop after sentence.

zherczeg · 2017-09-18T10:52:26Z

docs/12.EXT-REFERENCE-MODULE.md

 ```

 We can now load JavaScript files:
 ```c
 static const jerryx_module_resolver_t resolvers[] =
 {
  /* Consult the JerryScript module resolver first, in case the requested module is a compiled-in JerryScript module. */
-  jerryx_module_native_resolver,
+  &jerryx_module_native_resolver,


Why do you need & ?

Because jerryx_module_native_resolver is now a structure stored in a global variable, and this is an array of structure pointers, so I will update the declaration of resolvers a few lines up.

zherczeg · 2017-09-18T10:55:36Z

jerry-ext/module/module.c

+  bool return_value = false;
+
+  jerry_size_t c_module_name_size = jerry_get_utf8_string_size (name);
+  jerry_char_t c_module_name[c_module_name_size];


While this allocation is simple in source code level, I always has doubts about its efficiency. You need to move a lot of data on the stack.

So, should I use jmem_heap_alloc_block_null_on_error() instead?

I am just asking what is its cost. We could introduce convenience stuff for them (I saw you already created some macros for it).

zherczeg · 2017-09-18T10:56:16Z

jerry-ext/module/module.c

+  jerry_value_t ret = 0;
+  const jerryx_module_resolver_t *resolver_p;
+  jerry_value_t instances;
+  jerry_value_t canonical_names_static[5];


Lets use a define for this.

What do you mean "use a define"?

#define CANONICAL_NAMES_LOCAL_SIZE 4 or something. I think 4 is enough (probably 2 is the most frequent in practice).

zherczeg · 2017-09-18T10:56:39Z

jerry-ext/module/module.c

+
+  for (index = 0; index < count; index++)
+  {
+    canonical_names[index] = 0;


0? Not undefined?

zherczeg · 2017-09-18T10:57:07Z

jerry-ext/module/module.c

+    resolver_p = resolvers_p[index];
+    canonical_names[index] =
+    resolver_p->get_canonical_name == NULL ? jerry_acquire_value (name) :
+    resolver_p->get_canonical_name (name);


Interesting alignment.

If I move the else under jerry_acquire_value () I get this from check-vera.sh:

./jerry-ext/module/module.c:235: error: Indentation: 45 -> 4.

So, that's why it's like this.

Use round brackets.

martijnthe · 2017-09-18T12:44:46Z

jerry-ext/module/module.c

+jerryx_module_resolver_t jerryx_module_native_resolver =
+{
+  NULL,
+  jerryx_resolve_native_module


.get_canonical_name = NULL, .resolve = jerryx_resolve_native_module,

martijnthe · 2017-09-18T12:50:53Z

jerry-ext/include/jerryscript-ext/module.h

+{
+  jerry_value_t (*get_canonical_name) (const jerry_value_t name); /**< module name to canonicalize */
+  bool (*resolve) (const jerry_value_t canonical_name, /**< requested module's canonical name */
+                   jerry_value_t *result); /**< resulting module */


What's your rationale for passing around the name strings as jerry_value_ts?
I think most module resolvers will convert it right back to a C string.
Is it to avoid having to deal with memory management of the C strings?
(Probably a good reason since we can't assume malloc/free...)

Yeah, and I was actually thinking that, for path resolution, you might not even convert the whole value to a C string, but you would rather use fixed size chunks to gradually retrieve the string and maybe assemble the canonical name in a similarly gradual fashion. Then, unless you had a directory with a really, really, really long name, you wouldn't really be allocating all that much.

Call it streamed path resolution.

Another very important reason: the module cache is stored as a JavaScript object, so I have to convert a C string back to a jerry_value_t in order to check if that JavaScript object has a module at the module name's key.

Ah I see, yeah that makes sense.

gabrielschulhof · 2017-09-23T14:40:53Z

@martijnthe @LaszloLango @zherczeg I have updated the patch to reflect the changes to the moduling system.

zherczeg · 2017-09-25T07:59:08Z

docs/12.EXT-REFERENCE-MODULE.md

+{
+  jerry_value_t resulting_path;
+
+  /* Search name for "./" and "../", follow symlinks, etc., then create resulting_path via jerry_create_string (). */


We should emphasize here that the point is if two references points to the same physical file, it should be loaded only once.

zherczeg · 2017-09-25T08:05:37Z

jerry-ext/include/jerryscript-ext/module.h


 /**
 * Load a copy of a module into the current context using the provided module resolvers, or return one that was already
 * loaded if it is found.
 */
-jerry_value_t jerryx_module_resolve (const jerry_char_t *name, const jerryx_module_resolver_t *resolvers, size_t count);
+jerry_value_t jerryx_module_resolve (const jerry_value_t name, /**< module's name */


zherczeg · 2017-09-25T08:05:53Z

jerry-ext/include/jerryscript-ext/module.h

+{
+  jerry_value_t (*get_canonical_name) (const jerry_value_t name);  /**< module name to canonicalize */
+  bool (*resolve) (const jerry_value_t canonical_name,  /**< requested module's canonical name */
+                   jerry_value_t *result); /**< resulting module */


We do not use comments in header files.

zherczeg · 2017-09-25T08:06:56Z

jerry-ext/module/module.c

 {
  const jerryx_native_module_t *module_p = NULL;

+  jerry_size_t name_size = jerry_get_utf8_string_size (canonical_name);


Shall we call tostring on the name? Or check if it is a string.

We shouldn't need to. This function should not be called directly, even though a pointer to it is available. It should only be called by jerryx_module_resolve () and it should only ever receive a jerry_value_t stored in the array of canonical module names created by jerryx_module_resolve (). In fact, this function is not even documented. Only the structure jerryx_module_native_resolver is documented, and only the availability of it as a global data symbol is documented, not its contents.

zherczeg · 2017-09-25T08:08:52Z

jerry-ext/module/module.c

-jerryx_module_resolve (const jerry_char_t *name, /**< name of the module to load */
-                       const jerryx_module_resolver_t *resolvers_p, /**< list of resolvers */
+jerryx_module_resolve (const jerry_value_t name, /**< name of the module to load */
+                       const jerryx_module_resolver_t **resolvers_p, /**< list of resolvers */


** ? A pointer to a list of pointers which points to the resolvers? Why not an array of resolvers?

Because the resolvers are stored in-contiguously, and we do not want to make copies of them, right?

Are they used multiple times? Or why they are scattered?

I think one application use one resolver list, but this is not much of a code size increase, so lleave it be.

Resolvers may be declared global in separate .c files (jerryx_module_native_resolver certainly is), and they are collected into an array only once - the place that calls jerryx_module_resolve ().

zherczeg · 2017-09-25T08:10:15Z

jerry-ext/module/module.c

+    resolver_p = resolvers_p[index];
+    canonical_names[index] = ((resolver_p && resolver_p->get_canonical_name != NULL) ?
+      resolver_p->get_canonical_name (name):
+      jerry_acquire_value (name));


This is not how we align ?:

It's pretty tough not breaking 120 columns with this line. I'm still trying to figure out how to align it.

I renamed resolver_p to res_p then it fits.

I would move the condition into a bool value rather than shoretning the name.

zherczeg · 2017-09-25T08:11:09Z

tests/unit-ext/module/jerry-module-test.c

@@ -61,41 +61,61 @@ const char eval_string5[] =
 "}) ();";

 static bool
-resolve_differently_handled_module (const jerry_char_t *name,
+resolve_differently_handled_module (const jerry_value_t name,
                                                jerry_value_t *result)


Comment is missing, spaces are wrong.

gabrielschulhof · 2017-09-25T09:22:51Z

@zherczeg I have addressed your review comments.

gabrielschulhof · 2017-09-25T10:36:50Z

@zherczeg instead of a boolean I went straight for storing the function pointers in variables.

gabrielschulhof · 2017-09-25T10:46:49Z

... aaand I forgot to key the cache on the canonical name. Fixed now.

gabrielschulhof · 2017-09-25T13:45:51Z

@zherczeg could you or somebody with Travis access please restart the lint job? It seems to have been killed because it had stalled.

gabrielschulhof · 2017-09-25T16:37:24Z

Thanks!

LaszloLango · 2017-09-26T06:48:55Z

jerry-ext/include/jerryscript-ext/module.h

-  struct jerryx_native_module_t *next_p; /**< pointer to next module in the list */
+  const jerry_char_t *name_p;
+  const jerryx_native_module_on_resolve_t on_resolve;
+  struct jerryx_native_module_t *next_p;


Why did you remove the comments from here?

@zherczeg said we didn't add such /**< comments to the header files. Or did I misinterpret?

Yes, I think there was a misunderstanding here. We do not add comments to declarations, but only to definitions. The type definitions are usually in the header files, so we must put the comments there. Please don't remove these comments.

Sorry, I mean only for function arguments, not for types.

LaszloLango

Nice improvement

LaszloLango · 2017-09-26T06:54:20Z

jerry-ext/module/module.c

@@ -14,11 +14,13 @@
 */

 #include <string.h>
+#include "jmem.h"


I think we should not use internal headers in jerry-ext. I don't see what are you using from it. Please remove it if possible.

LaszloLango · 2017-09-26T06:56:03Z

jerry-ext/include/jerryscript-ext/module.h

-bool jerryx_module_native_resolver (const jerry_char_t *name, jerry_value_t *result);
+typedef struct
+{
+  jerry_value_t (*get_canonical_name) (const jerry_value_t name);


Missing comments

I need to know for sure. Do we, or do we not add /**< comments after structure members and function arguments? How do we document function pointer structure members, since the formal parameters of the function signature also needs to be documented?

gabrielschulhof · 2017-09-26T12:19:05Z

@LaszloLango fixed up the comments and added some _ps.

zherczeg

LGTM with some style fix.

zherczeg · 2017-09-26T12:30:41Z

jerry-ext/include/jerryscript-ext/module.h

+ */
+typedef jerry_value_t (*jerryx_module_get_canonical_name_t) (const jerry_value_t name); /**< The name for which to
+                                                                                         * compute the canonical
+                                                                                         * name */


Align the start of the comment lines to the same column.

zherczeg · 2017-09-26T12:31:00Z

jerry-ext/include/jerryscript-ext/module.h

-bool jerryx_module_native_resolver (const jerry_char_t *name, jerry_value_t *result);
+typedef bool (*jerryx_module_resolve_t) (const jerry_value_t canonical_name, /**< The module's canonical name */
+                                         jerry_value_t *result); /**< The resulting module, if the function returns
+                                                                  * true */


Ditto. And similar to below.

zherczeg · 2017-09-26T12:34:04Z

tests/unit-ext/test-ext-module-canonical.c

+
+  return (!strcmp ((char *) name_string, "batman") ? jerry_acquire_value (name):
+         (!strcmp ((char *) name_string, "bruce-wayne") ? jerry_create_string ((jerry_char_t *) "batman"):
+         jerry_create_undefined ()));


Fix the style of these lines.

zherczeg · 2017-09-26T12:34:30Z

tests/unit-ext/test-ext-module-canonical.c

+  jerry_init (JERRY_INIT_EMPTY);
+
+  jerry_value_t batman = jerry_create_string ((jerry_char_t *) "batman");
+  jerry_value_t bruce_wayne = jerry_create_string ((jerry_char_t *) "bruce-wayne");


I hope these are not copyrighted.

Good point. I'll use Liberator, which is public domain.

They are still, however, "use at your own risk" characters. - this doesn't sound entirely public

sigh Alice and Bob it is.

LaszloLango

LGTM after the style fixes.

gabrielschulhof · 2017-09-26T14:26:14Z

@zherczeg @LaszloLango please have another look. I updated the test to use #define-ed strings to store the alias and the actual name at the top, and I replaced the ternary composition with if/else if/else. I also changed the resolver initializer to use the { .member_name = value } notation.

Before attempting to load a module, each provided resolver must be given an opportunity to examine the name of the requested module without actually loading it so as to canonicalize it, in case a module can be referred to by multiple names. Then, modules are loaded and cached by their canonical name. JerryScript-DCO-1.0-Signed-off-by: Gabriel Schulhof [email protected]

LaszloLango · 2017-09-27T06:43:56Z

still LGTM

zherczeg · 2017-09-29T10:02:23Z

Still ok.

…ipt-project#2013) Before attempting to load a module, each provided resolver must be given an opportunity to examine the name of the requested module without actually loading it so as to canonicalize it, in case a module can be referred to by multiple names. Then, modules are loaded and cached by their canonical name. JerryScript-DCO-1.0-Signed-off-by: Gabriel Schulhof [email protected]

gabrielschulhof force-pushed the module-canonical-name branch 2 times, most recently from c9cb700 to 4b53e14 Compare September 17, 2017 21:29

LaszloLango requested changes Sep 18, 2017

View reviewed changes

gabrielschulhof force-pushed the module-canonical-name branch from 4b53e14 to 9faffed Compare September 18, 2017 10:31

zherczeg reviewed Sep 18, 2017

View reviewed changes

gabrielschulhof force-pushed the module-canonical-name branch from 9faffed to fd0cbe4 Compare September 18, 2017 11:30

martijnthe reviewed Sep 18, 2017

View reviewed changes

gabrielschulhof force-pushed the module-canonical-name branch 2 times, most recently from cd7e880 to ac23faa Compare September 23, 2017 14:40

zherczeg requested changes Sep 25, 2017

View reviewed changes

gabrielschulhof force-pushed the module-canonical-name branch from ac23faa to 0774aed Compare September 25, 2017 09:21

gabrielschulhof force-pushed the module-canonical-name branch 2 times, most recently from 702fdd4 to 45dbd81 Compare September 25, 2017 10:31

gabrielschulhof force-pushed the module-canonical-name branch from 45dbd81 to 2ff4b5f Compare September 25, 2017 10:46

LaszloLango reviewed Sep 26, 2017

View reviewed changes

LaszloLango requested changes Sep 26, 2017

View reviewed changes

gabrielschulhof force-pushed the module-canonical-name branch from 2ff4b5f to d106ad4 Compare September 26, 2017 12:18

zherczeg approved these changes Sep 26, 2017

View reviewed changes

LaszloLango approved these changes Sep 26, 2017

View reviewed changes

gabrielschulhof force-pushed the module-canonical-name branch from d106ad4 to 8f9dc5e Compare September 26, 2017 14:23

gabrielschulhof force-pushed the module-canonical-name branch from 8f9dc5e to b0fe0bf Compare September 26, 2017 14:28

zherczeg merged commit 6d53931 into jerryscript-project:master Sep 29, 2017

gabrielschulhof mentioned this pull request Jan 10, 2018

Node.js 10.0.0 Tracking nodejs/Release#291

Closed

gabrielschulhof deleted the module-canonical-name branch June 18, 2019 00:08

module extension: add support for canonical name resolution #2013

module extension: add support for canonical name resolution #2013

Uh oh!

Conversation

gabrielschulhof commented Sep 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Sep 18, 2017

Uh oh!

zherczeg 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

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!

martijnthe Sep 18, 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!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Sep 23, 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!

gabrielschulhof Sep 25, 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

martijnthe Sep 18, 2017 •

edited

Loading

gabrielschulhof Sep 25, 2017 •

edited

Loading