Add JS_IsProxy, JS_GetProxyHandler and JS_GetProxyTarget

[bnoordhuis]

This commit also turns JS_IsArray into a simple predicate function. It no longer punches automatically through proxies because that can raise exceptions and is inconsistent with the other predicate functions.

Fixes: #938

[xeioex]

Hi @saghul,

It no longer punches automatically through proxies because that can raise exceptions and is inconsistent with the other predicate functions.

Is it a good justification for changing JS_IsArray() API? Why not change the behavior but leave the API intact?

Changes like this make it harder to use the library. Now, the library users had to use not very fun versioning checks.

For example, in njs we are trying to work with both quickjs and quickjs-ng. There are already some changes, it is OK. I understand that some changes in API are necessary. But I hope to see less changes without good justification.

[saghul]

I find the justification to be good.

Also, look at the progress, well, the lack thereof, of bellard/quickjs. We won't be beholden to it since no development is going on.

Why do you want to support both? There are tons of APIs missing so how do you handle that?

[xeioex]

@saghul

I find the justification to be good.

Could you please elaborate? What do you suggest to do in such a case for current quickjs-ng users?

[xeioex]

@saghul

Why do you want to support both? There are tons of APIs missing so how do you handle that?

We will have to switch to NG eventually, but recent rapid changed in NG are bit scary. It seems we have to wait now, until the code somewhat is settled. Therefore we staying at quickjs for now, but we want to be compatible with NG as well. At least in CI.

[saghul]

@saghul

I find the justification to be good.

Could you please elaborate? What do you suggest to do in such a case for current quickjs-ng users?

1486c85

You can use this.

[saghul]

@saghul

Why do you want to support both? There are tons of APIs missing so how do you handle that?

We will have to switch to NG eventually, but recent rapid changed in NG are bit scary. It seems we have to wait now, until the code somewhat is settled. Therefore we staying at quickjs for now, but we want to be compatible with NG as well. At least in CI.

Help me understand. What is scary and what needs to settle? NG has been going on for over a year now and literally every release has been a net improvement over the original IMHO.

[xeioex]

@saghul

You can use this.

Yes, something like this will work. That is what I called "not very fun versioning checks". Could you please address the original question?

[saghul]

Aren't you already neck deep in checks since we have a good number of new APIs you might want to use? Even the build system is different 😅

[xeioex]

@saghul

Help me understand. What is scary and what needs to settle?

Sure.

Something like Adding and removing inline caches come to mind.
Or adding and reverting changes, like 9c5c441.
Breaking existing API, like with JS_IsArray(). Gives me feeling of a rushed development, I might be wrong.

NG has been going on for over a year now and literally every release has been a net improvement over the original IMHO.

I am not denying that. My only emphasis is that, while the project is moving in the right direction it will be even more fun for user not to fix the existing code.

[xeioex]

@saghul

Aren't you already neck deep in checks since we have a good number of new APIs you might want to use? Even the build system is different 😅

Surprisingly no. So far we have to test only for JS_IsSameValue(cx, a, b) vs JS_SameValue(cx, a, b).

[bnoordhuis]

Breaking existing API, like with JS_IsArray()

Cohesiveness is important. The old JS_IsArray wasn't. There's one less pitfall to remember now.

You're programming against the API. Wouldn't you agree it's better to have fewer exceptions to the rules rather than more?

[xeioex]

@bnoordhuis

The old JS_IsArray wasn't. There's one less pitfall to remember now.

Why not change the behavior, but not the signature?

Cohesiveness is important.

It seems that the following functions can work without ctx too.

static inline bool JS_IsBigInt(JSContext *ctx, JSValue v)
JS_EXTERN bool JS_IsError(JSContext *ctx, JSValue val);
JS_EXTERN bool JS_IsUncatchableError(JSContext* ctx, JSValue val);
JS_EXTERN bool JS_IsConstructor(JSContext* ctx, JSValue val);

Wouldn't you agree it's better to have fewer exceptions to the rules rather than more?

Help me understand, how for example, JS_IsArray() is categorically different from JS_IsFunction(JSContext* ctx, JSValue val) where ctx is required?

My overall point is: Yes, sometimes changes in API are necessary. But if they are done, they should be done systematically and at once. If there is no clear criteria, probably the change does not improve situation much and is less justified.

[bnoordhuis]

Who says we're done making changes? Except for JS_IsFunction those predicates don't use ctx.

[chqrlie]

It seems that the following functions can work without ctx too.

static inline bool JS_IsBigInt(JSContext *ctx, JSValue v)
JS_EXTERN bool JS_IsError(JSContext *ctx, JSValue val);
JS_EXTERN bool JS_IsUncatchableError(JSContext* ctx, JSValue val);
JS_EXTERN bool JS_IsConstructor(JSContext* ctx, JSValue val);

What is the benefit in removing the ctx argument? ctx must be available in the caller to do anything useful with the object and the performance impact is negligible. Conversely omitting the ctx from all published predicates for which our code does not need it makes it impossible to later add it back for any potential good reason we cannot think of right now (for example keeping stats on API calls, but better ones might come to mind later).

This also does not stop us from using internal functions that omit the argument are are perfect candidates for compile time or link time inlining.

My take is it is probably acceptable for JS_IsArray to no longer walk the proxy chain, but a more important API change such as removing the ctx argument is unwarranted.

[bnoordhuis]

What is the benefit in removing the ctx argument?

Consistency and efficiency. Consistency I've elaborated above.

Efficiency: calls to e.g. JS_DupValue() force the compiler to shuffle registers in the right order (say rdi and rsi), but then JS_DupValue() completely ignores rdi. That's doubly offensive when the old thing in rdi is spilled to the stack and reloaded afterwards.

In isolation maybe not so bad, and LTO can potentially (if not always in actuality) claw back some of that performance loss, but it's one of those death by a 1,000 papercuts things.

[hongzhidao]

Hi,
Good talk, I'd like to add my two cents.

1. There is a JS function Array.isArray() which calls JS_IsArray() internally. It would be great to keep C and JS having the same behavior, somethings script language like JS is a way to simplify C development.
   I mean translating JS code into C code as easily as possible. For example:

/* both of a and b are JSValue internally */
Array.isArray(a);
JS_IsArray(ctx, b);

2. Now with the change, there are JS_IsArray(), js_is_array(), and Array.isArray(), but their implementations are not equal. It might introduce confusion.
3. I wonder when to use JS_IsProxy(). If we want to check IsArray with proxy, we need to do it with the new API and existing JS_IsArray(), right?

Thanks.

[xeioex]

@bnoordhuis

Who says we're done making changes? Except for JS_IsFunction those predicates don't use ctx.

That is what I am talking about. Making changes non-systematically makes it harder for users to use the library.

[xeioex]

@bnoordhuis

Efficiency: calls to e.g. JS_DupValue() force the compiler to shuffle registers in the right order (say rdi and rsi), but then JS_DupValue() completely ignores rdi. That's doubly offensive when the old thing in rdi is spilled to the stack and reloaded afterwards.

That is speculation, isn't it?. Let's start from the question "could we even measure the change with statistical significance".

From my experience, memory access pattern is far more significant, than the number of register spills (L1 cache). The later can be even unmeasurable. Meaning ruining just one core struct alignment could negatively impact performance far more. Registers are fast, cache is fast, memory is ~100x slower.

[bnoordhuis]

@hongzhidao If you want the old behavior, use JS_IsProxy() + JS_GetProxyTarget() and JS_IsArray().

@xeioex It's turning into an uninteresting discussion. Best stop now.

[xeioex]

@bnoordhuis

@xeioex It's turning into an uninteresting discussion. Best stop now.

That is sad to hear that performance question is uninterested one.

What I wanted to hear is that the feedback from the users was taken and valid concerns are addressed/admitted.
I am all for compromises and seeing each other points. I appreciate what you guys are doing. I also think this is in the best interest of quickjs-ng to hear from the community, so everybody can flourish. Sorry if my criticism was too direct. No harm was intended.