Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Array.fromAsync #975

Merged
merged 3 commits into from
Mar 15, 2025
Merged

Add Array.fromAsync #975

merged 3 commits into from
Mar 15, 2025

Conversation

bnoordhuis
Copy link
Contributor

Implement Array.fromAsync as a lazily loaded bytecode function.

Fixes: #962

Implement Array.fromAsync as a lazily loaded bytecode function.

Fixes: quickjs-ng#962
Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some questions!

/* return the value associated to the autoinit property or an exception */
typedef JSValue JSAutoInitFunc(JSContext *ctx, JSObject *p, JSAtom atom, void *opaque);

static JSAutoInitFunc *const js_autoinit_func_table[] = {
js_instantiate_prototype, /* JS_AUTOINIT_ID_PROTOTYPE */
js_module_ns_autoinit, /* JS_AUTOINIT_ID_MODULE_NS */
JS_InstantiateFunctionListItem2, /* JS_AUTOINIT_ID_PROP */
js_bytecode_autoinit, /* JS_AUTOINIT_ID_BYTECODE */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this get initialized? At runtime start?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when the context is initialized, in JS_AddIntrinsicBaseObjects.

@@ -7383,13 +7394,61 @@ int JS_IsInstanceOf(JSContext *ctx, JSValueConst val, JSValueConst obj)
return JS_OrdinaryIsInstanceOf(ctx, val, obj);
}

#include "gen/builtin-array-fromasync.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of this TBH. I was wondering if the following makes any sense:

  • have the JS code as a string in the C code
  • lazily byte-compile and eval it, ideallly on first use
  • profit?

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better approach would be to compile this and possibly other APIs to bytecode as part of the build process and embed said bytecode in the interpreter binary. The APIs would still need to be instantiated lazily upon use but they would be available even if the compiler code is not linked in an embedded target.

Copy link
Contributor Author

@bnoordhuis bnoordhuis Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have the JS code as a string in the C code [..] profit?

No profit, only loss. Takes more work at runtime and embedding the source is twice as big as the bytecode.

@chqrlie I don't understand your comment vis-a-vis embedded target. edit: oh, I do - that was addressing Saúl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better approach would be to compile this and possibly other APIs to bytecode as part of the build process and embed said bytecode in the interpreter binary

Doing it as part of the build results in a chicken and egg problem though. I have a lot of experience with the way V8 does two stage builds and it's miserable.

The approach I picked here (generate ahead of time and commit artifact) results in the least amount of ongoing work, I'm fairly sure.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was possible to create smaller executables with embedded byte code but without the eval function, hence without the compiler code. This should still be possible with the current code, with manual C compilation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include "gen/builtin-array-fromasync.h"

This implies that future built-in bytecode functions will be placed in separate files.
Would it be possible to concatenate them into a single file (say, builtins.h) instead? That would keep copy-paste vendoring easier, as we'd have to copy just one new file instead of N new files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all in a single file if you use the amalgamated build. If that doesn't work for you, let me know; I'm not going to say yes or no straight away but I can take it into consideration.

FWIW, it's probably academic for the foreseeable future. I don't see us adding too many builtins in this way, only ones that would be extremely onerous to write in C.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've considered switching to the amalgamation, the problem is that it would make patching too inconvenient.
Right now, I can just develop and commit patches on top of my vendored copy; with the amalgamation I'd have to run a separate build step to even test a patch, then also maintain a separate repository to publish the patched sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that? In my own projects I vendor the amalgamation and float any patches I carry on top of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean to develop/apply the patches on top of the amalgamation? I guess the main drawback is that it becomes harder to borrow/upstream them.
(I feel uneasy about manually editing autogenerated artifacts in general; e.g. if the script changed the order of files on update, I'd have to manually apply everything again. But maybe my fears are unfounded.)

Having typed this out, I've just remembered another thing: I have a binary that depends on libregexp and only libregexp, so I'd also have to patch the generator script to support this somehow.

@richarddd
Copy link
Contributor

And this would be to complex to build in native code?

@saghul
Copy link
Contributor

saghul commented Mar 14, 2025

And this would be to complex to build in native code?

Yeah, very. Specially the awaits.

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to eventually find a way to not have the extra header, but I'm good for now!

@bnoordhuis bnoordhuis merged commit bfc9e3c into quickjs-ng:master Mar 15, 2025
128 checks passed
@bnoordhuis bnoordhuis deleted the fix962 branch March 15, 2025 21:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array.fromAsync
5 participants