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 JS_NewObjectFrom and JS_NewObjectFromStr #871

Merged
merged 6 commits into from
Feb 2, 2025

Conversation

bnoordhuis
Copy link
Contributor

I did it as a sequence of commits that iteratively refine the logic so you can see how it builds up from the naive version to the final version.

The final version is definitely quite a bit faster vs. JS_NewObject + JS_SetProperty because we can make some assumptions about the object shape and skip a lot of repeated busywork.

I do wonder though how often I, as a quickjs user, would actually use these functions. I couldn't find many places in qjs.c or run-test262.c where it made sense. The change I made to the latter is admittedly somewhat contrived, so... WDYT?

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.

At a quick glance there are a couple of places in txiki.js where I could use this. Not any hot path but still.

@xeioex
Copy link
Contributor

xeioex commented Feb 2, 2025

I can see a couple of places where it can be useful.

Have you considered the interface like this

JSValue JS_NewObjectFrom(JSContext *ctx, ...) via va_arg() macro?

The usage:

obj = JS_NewObjectFrom(ctx, prop1, val1, prop2, val2, NULL);

@bnoordhuis
Copy link
Contributor Author

Yes, I considered that but rejected it because:

  1. Inflexible. You cannot programmatically build up lists of properties that way

  2. Inefficient. Termination with a sentinel value is mutually exclusive with optimizing the object shape (don't know how many properties to make room for) but taking a count parameter makes the API error prone to use

@bnoordhuis bnoordhuis merged commit 26581b9 into quickjs-ng:master Feb 2, 2025
61 checks passed
@bnoordhuis bnoordhuis deleted the new-object-from branch February 2, 2025 09:40
# 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.

3 participants