Review inlined functions in quickjs.h

[saghul]

Some of them are just inline some others use a way to force inlining, but my understanding is that it's still not 100% guaranteed.

Also, due to things like:

JS_EXTERN void __JS_FreeValue(JSContext *ctx, JSValue v);
static inline void JS_FreeValue(JSContext *ctx, JSValue v)
{
    if (JS_VALUE_HAS_REF_COUNT(v)) {
        JSRefCountHeader *p = (JSRefCountHeader *)JS_VALUE_GET_PTR(v);
        if (--p->ref_count <= 0) {
            __JS_FreeValue(ctx, v);
        }
    }
}

We need to declare __JS_FreeValue as part of the API...

Is there a better way around this?

Also, for functions like this:

static js_force_inline JSValue JS_NewBool(JSContext *ctx, JS_BOOL val)
{
    return JS_MKVAL(JS_TAG_BOOL, (val != 0));
}

Would moving them to the C file an relying on compiler smarts be bad?

What would be the right way to benchmark this?

[saghul]

@chqrlie Any thoughts on this?

[chqrlie]

There is substantial cleanup to do here:

• we should avoid APIs starting with __
• we should minimize exposure of the internal implementation.
• we might add a quickjs-internal.h header for extension implementors/hackers who want deeper hooks into the implementation but may need to recompile/adapt when the implementation changes.
• I have a patch almost ready to get rid of __JS_NewFloat64, make JS_NewFloat64 always create a floating point value and add JS_NewDouble to create an integer if possible. I am not satisfied with this name, maybe JS_NewNumber would be better.

Regarding the inline functions that are not simple wrappers such as JS_NewBoolean, we should probably make them regular APIs, implement them in quickjs.c and use local static inline implementations to avoid overhead in the engine. Compiling with link-time-optimisation may make this irrelevant as the linker will inline whatever seems appropriate, but generating dynamic libraries definitely add some overhead for API entry points. Not sure how to benchmark this as we don't really have extensive code calling through the API. Do you have benchmarks in txiki.js?

[saghul]

• we should avoid APIs starting with __

Agreed.

• we should minimize exposure of the internal implementation.

Agreed.

• we might add a quickjs-internal.h header for extension implementors/hackers who want deeper hooks into the implementation but may need to recompile/adapt when the implementation changes.

Not sure what we'd put in there, at least for now...

• I have a patch almost ready to get rid of __JS_NewFloat64, make JS_NewFloat64 always create a floating point value and add JS_NewDouble to create an integer if possible. I am not satisfied with this name, maybe JS_NewNumber would be better.

That's a great start! I do like JS_NewNumber.

Regarding the inline functions that are not simple wrappers such as JS_NewBoolean, we should probably make them regular APIs, implement them in quickjs.c and use local static inline implementations to avoid overhead in the engine.

I can send a few patches around that.

Not sure how to benchmark this as we don't really have extensive code calling through the API. Do you have benchmarks in txiki.js?

Not really, since I basically call into QuickJS. I do statically compile it too...