Refactor JSClosure to leverage FinalizationRegistry

[j-f1]

• closure.release() and JSOneshotClosure are no longer needed
• FinalizationRegistry calls swjs_free_host_function to remove the corresponding Swift closure from the global dictionary.
• deallocating a JSClosure removes the JS function from the heap. If the function isn’t being held somewhere else (such as inside an event handler), it will become eligible for garbage collection, and the Swift closure will be eventually released.
• JavaScriptHostFuncRef is now obtained from a global “expando” variable because JSClosure { ... }; JSClosure { ... } may reuse an ObjectIdentifier. If we decide to add multithreading, we should be careful to only modify closureRef using a lock.
• TypeScript has been updated to add support for WeakRef/FinalizationRegistry

Fixes #106.

[MaxDesiatov]

I wonder if there's a good way to feature-flag it? It would be great to dynamically detect if FinalizationRegistry is available and to trigger different codepaths based on that. Otherwise merging this means projects depending on JSKit will break on browsers older than Safari 14.1.

This is another nudge towards enabling browser feature detection/versioning in carton and maybe even Swift itself. It certainly would be handy to reuse infrastructure we already have for targeting multiple iOS versions. Then people who target only new browsers would have unused stuff stripped out, while people deploying to old browsers would have a way to handle conditional feature support.

[j-f1]

I don’t think so — it would require substantially different API usage on the Swift side if it was unavailable unless we just wanted to leak memory on the Swift side. I’m not too concerned about compatibility because recent versions of most evergreen browsers support it, and Safari 14.1 is available on Mojave (10.14) and above, so most people should be OK with this, especially given the rate at which people are using SwiftWasm right now 😉

[MaxDesiatov]

I understand, my concern is with economic accessibility here. There are enough old devices that will never get Safari 14.1 installed on them, and I would love Tokamak to support them. We also know some people researching Tokamak for use in production environment right now, that puts some constraints on browsers and devices compatibility too.

[j-f1]

I guess I’m not sure what the target audience would be. Mixpanel’s iOS adoption report shows that 90% of devices run iOS 14, and only 5% of devices run a version below iOS 13. Chrome seems to run on Android 5 and above (although I’m not 100% positive they will actually give you the latest version).

Two other things to consider: First, SwiftWasm’s current build output is fairly large, making it difficult to use on mobile data plans or with cheaper phones. Second, I doubt the pool of devices that support SwiftWasm but not FinalizationRegistry is growing.

[MaxDesiatov]

After asking around, I was surprised to hear that requiring latest iOS is not such a problem for some devs. I'm happy to defer to Yuta on this after all.

[kateinoigakukun]

Overall +1 👍 I want to merge change with feature flag like -D JAVASCRIPTKIT_WITHOUT_WEAKREF as an escape hatch. What do you think about this?

[kateinoigakukun]

Could you measure runtime performance between JSOneshotClosure and JSClosure for deallocation overhead?
I think deallocation that depends on GC cycle will increase memory usage while waiting until event loop frame.

(But I'm not sure how this overhead affects users experience 😅 )

[kateinoigakukun]

So JSClosure in browsers without FinalizationRegistry support won't be deallocated, right?

[j-f1]

If FinalizationRegistry is not supported, the JS side will crash on startup when it attempts to call new FinalizationRegistry().

[j-f1]

I‘d be happy to merge with a feature flag, but I’m not sure how to implement it on the JS side. I guess I could conditionally construct the FinalizationRegistry if a special API call (like the existing version call) returns true

[kateinoigakukun]

@j-f1 I think we can expose an API to get feature bit-flags from Swift side like: func swjs_feature_flags() -> UInt32

[nmn]

Just chiming in:

Instead of a flag, there should a separate API for NoGCJSClosure. If someone really wants to support older browsers, they'll need to write different Swift code anyway, so let them choose a different API.

Further, I don't think this is really a concern at all. In a few short months Safari 15 will be out Safari 14.1 will be the "old" version. It should be considered very safe to assume minimum Safari 14.1

[MaxDesiatov]

Think I'm fine proceeding with this approach after all without supporting older browsers.

@kateinoigakukun do you think switching to FinalizationRegistry would have any impact on async/await support? Or do we need to prove that FinalizationRegistry does not have any performance impact first?

[github-actions]

Time Change: +1,909.75ms (18%) ⚠️

Total Time: 10,482ms

Test name	Duration	Change
Serialization/Write JavaScript number directly	226.25ms	+63.5ms (28%)	🚨
Serialization/Write JavaScript string directly	197ms	+30ms (15%)	⚠️
Serialization/Swift Int to JavaScript	3,346.25ms	+682ms (20%)	🚨
Serialization/Swift String to JavaScript	3,527.75ms	+754.75ms (21%)	🚨
Object heap/Increment and decrement RC	3,184.75ms	+379.5ms (11%)	⚠️

_{performance-action}

[j-f1]

My recent updates to the JS side cause the FinalizationRegistry to always be initialized — if the plan is to drop support for older browsers I think c77e069 (#128) can be reverted to officially remove the legacy closures. Otherwise, I’d have to make some changes to ensure the code continues to run on older runtimes.

[MaxDesiatov]

@j-f1 what do you think would be the best way to benchmark how FinalizationRegistry behaves against older closures API? Do we already have any benchmarks for either of these?

[j-f1]

I don’t think benchmarks are the right way to evaluate this change. The primary result is to improve safety/ease of use by tying the lifetime of the Swift closure to that of the JS function. Since the benchmarks run above show no major performance changes, I’m not personally super concerned.

[MaxDesiatov]

Right, I was only interested if it was worth dropping manually-managed version altogether in favor of the garbage-collected one. @kateinoigakukun WDYT?

[kateinoigakukun]

Could you try this branch on Tokamak and ensure no compatibility issues?
I confirmed it by myself, and it works well 👍