Skip to content
New issue

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

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

Already on GitHub? Sign in 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?

quickjs.c Show resolved Hide resolved
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants