Add JS-side cache of wasm table entries. NFC

[sbc100]

Split out from #13844, this is an internal optimization
that avoids repeated calls to table.get.

[sbc100]

These new functions are are all designed to be internal-only library functions.. but we don't currently have a good way to mark them as such. Perhaps I should at least document them as such?

[sbc100]

Once we figure solve #15242 we can mark these are internal-only members.

[kripken]

This approach sgtm.

Overall it is like #14534 but with a new API instead of a "polyfill". I imagine this is more compact in code size, but that might be worth verifying.

[kripken]

Code also looks good.

[sbc100]

I'll wait for @juj since this is basically his code split into a separate PR.

[juj]

Thanks, lgtm

[sbc100]

I think I will wait for #15294 to land so I can mark these new functions as __internal

[kripken]

@jpharvey this should address the perf issues you mentioned in #14534

[juj]

I don't think setWasmTableEntry and getWasmTableEntry should be marked internal? This promotes the scenario where our internal library_x.js files use APIs that are not enabled for end users to utilize, which is not good design. (e.g. the embind.js and library.js changes above)

End user JS libraries should be at the "same power level" as our own core-provided libraries whenever possible, because a lot of users use these libraries to mimic their own libraries off of. That will avoid us from building up issues like we have e.g. with the existence of the deps_info.py script.

[sbc100]

Sure, if you think we want to commit to maintaining these APIs in the long term, and they are not just an internal detail then we should make them public.

However, I think we should always try to keep things internal where possible to avoid API surface creep, which make it hard for us to evolve or remove APIs over time.

The whole point of making certain JS library functions internal is to limit the power to user-level JS library file to see our internal symbols. There are inherently two tiers of exposure here by design.

Currently the use of an internal symbol only generates a warning.. so folks can still use internal symbols today.

[sbc100]

In my mind the point of marking something as internal is to prevent external JS library files from depending on something that we might want to change or remove in the future. So this is really a decision we need to make a on per-symbol basis, right?

I'm OK with making these particular symbols as externally visible, but I'm wonder what you think would make a good criteria for an internal symbol? Perhaps, as in this case we could say that if we used a symbol in more than one library file it should be considered externally visible/useful?

[juj]

Sure, if you think we want to commit to maintaining these APIs in the long term,

I mean, what API should a JS developer then use to invoke a wasm function pointer from JS if not getWasmTableEntry?

[sbc100]

Sure, if you think we want to commit to maintaining these APIs in the long term,

I mean, what API should a JS developer then use to invoke a wasm function pointer from JS if not getWasmTableEntry?

This change was not intended to be an API change, just and internal optimization.

The existing makeDynCaller / dynCall APIs are still public and did not change. We also (IIUC) have a new public API coming in the form of your wbind, right?

[juj]

I'll look at adding the wbind() API then.

[sbc100]

I kind of like the internal symmetry of getWasmTableEntry/setWasmTableEntry. Could wbind not just be an external-facing alias of getWasmTableEntry?

[juj]

I am not so hot on exposing two functions that do the same. Having internal library code call getWasmTableEntry and public API to use wbind that do the same does not make sense.

Also I don't think that the symmetry of getWasmTableEntry/setWasmTableEntry is particularly interesting, because getWasmTableEntry is a public API function, whereas setWasmTableEntry is an internal implementation API? (afaik there is never a reason for external users to call it, is there?)

Another option is that we drop the plan for the wbind alias, and expose getWasmTableEntry as the public facing API?

[sbc100]

I guess thats not a bad option... I find getWasmTableEntry to be more explict about what its doing than wbind which makes me things of embind and other bindings systems. (But then again we had this debate over on #13844 and I think I gave on finding a better name.. but if you are OK with getWasmTableEntry I think we could go for it).. @kripken WDYT about this being the public name for this?

[kripken]

I agree getWasmTableEntry sounds like a reasonable API name. It is clear and unambiguous.