chore: move JS_{Dup,Free}Value and the RT variants from header, reduced duplication

[richarddd]

js_dup and JS_DupValue is the same function expect for different parameters. However the ctx parameter is not used.

Similarly a few functions have unused CTX parameter.

This PR removes duplicate code by using JS_DupValue instead of js_dup and also removing unused ctx parameter in a few other functions.

[saghul]

Not a fan of this change. Those are very used functions, and this would break a lof of code out there that relies on them for no good reason.

[richarddd]

Not a fan of this change. Those are very used functions, and this would break a lof of code out there that relies on them for no good reason.

Indeed, this is a breaking change. But also not good to duplicate functionality and force use of unused parameters. Another approach here to maintain signatures would be to call js_dup from JS_DupValue.

[saghul]

But also not good to duplicate functionality

It's so small that I don't see a problem in this case.

force use of unused parameters.

We are already neck deep on that one, hence the compiler flags to disable the warnings, so hardly an argument here.

Another approach here to maintain signatures would be to call js_dup from JS_DupValue.

That would mean js_dup would be part of the API, which sounds wrong. Or, we move JS_{Dup,Free}Value and the RT variants from the header file to the C file and then js_dup can remain out of the API.

The latter would also allow us to not expose JSRefCountHeader, which is a good thing I guess.

Oh and also __JS_FreeValue, not having to expose that would be nice.

How about we measure if there is a performance impact from moving the functions to the implementation file (and thus potentially lose the inlining) and take it from there?

[richarddd]

It's so small that I don't see a problem in this case.

True but bugs could very easily by introduced here if we now need to maintain two copies of the same functionality. I would argo its not trivially small.

We are already neck deep on that one, hence the compiler flags to disable the warnings, so hardly an argument here.

I assume a lot of these are kept for compatibility reasons, but shouldn't we strive for removal of these? Unused params is usually a bit of a code smell and can prevent future refactoring or optimizations.

That would mean js_dup would be part of the API, which sounds wrong. Or, we move JS_{Dup,Free}Value and the RT variants from the header file to the C file and then js_dup can remain out of the API.

Yes, that would by my suggestion as well as we naturally don't want to expose internal functions in the header.

The latter would also allow us to not expose JSRefCountHeader, which is a good thing I guess.

Oh and also __JS_FreeValue, not having to expose that would be nice.

How about we measure if there is a performance impact from moving the functions to the implementation file (and thus potentially lose the inlining) and take it from there?

I can test and see. Would you expect regression due to removal of inlining? AFAIK inline keyword is a suggestion only and i still determined by the compiler right?

[saghul]

I can test and see. Would you expect regression due to removal of inlining? AFAIK inline keyword is a suggestion only and i still determined by the compiler right?

Let's! Yep, but IIRC Ben noted there a ton of dup/free calls happening, so better measure to see if the impact is real.

[saghul]

I assume a lot of these are kept for compatibility reasons, but shouldn't we strive for removal of these? Unused params is usually a bit of a code smell and can prevent future refactoring or optimizations.

Not only. For example there are tons of unused argc / argv specially for functions that don't use parameters. We could silence the hell out of those, but I suspect it would be a tedious process for no huge gain...

[richarddd]

I tested and saw 0 regression in performance. Same score for 3 test runs of v8-v7 test.

[bnoordhuis]

Did you try tests/microbench.js? The cost of dup/free calls should be very noticeable there; it adjusts refcounts well over a billion times.

[richarddd]

Did you try tests/microbench.js? The cost of dup/free calls should be very noticeable there; it adjusts refcounts well over a billion times.

Tested and i saw about a 0.7% improvement actually. This is probably within error margin

new:
run 1: total 6649.17
run 2: total 6638.69

current:
run 1: total 6604.48
run 2: total 6603.82

[saghul]

That's encouraging! I'd go for it. @bnoordhuis WDYT?

[saghul]

I like how this turned out. I'd like @bnoordhuis 's stamp before we merge.