-
-
Notifications
You must be signed in to change notification settings - Fork 51
Suggestion for #128: Add escape hatch for old environments #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5a7fed5
to
732b0a1
Compare
Time Change: -265.75ms (2%) Total Time: 8,862.25ms
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, thanks!
Hmm, which line introduced performance regression... 🤔 |
Woah, yeah, that seems pretty serious |
732b0a1
to
592853f
Compare
The serious regression appeared only on JavaScript cases. So no regression in our codebase, I hope. |
What are you referring to by "JavaScript cases" here? |
Here JavaScriptKit/IntegrationTests/bin/benchmark-tests.js Lines 25 to 40 in 43a33ee
|
So you mean that |
I guess that's right. But not sure why this regression didn't happen on the |
I couldn't see considerable bench regression on my end |
Maybe it was a temporary CI glitch then? |
// Create a new JavaScript function which calls the given Swift function. | ||
// 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`. | ||
super.init(id: 0) | ||
let objectId = ObjectIdentifier(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched away from using ObjectIdentifier
because:
foo(JSClosure { _ in "this closure" })
bar(JSClosure { _ in "and this closure" })
…ended up having the same ObjectIdentifier
value. The monotonically increasing global ID works around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that behavior seems natural. In what use cases is that behavior useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding multiple event listeners to a DOM node, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, the closure instances are retained by host environment, so the Swift instances are also retained and they should have different identifies at a time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closure itself ({ _ in ... }
) is retained by the host environment, but the JSClosure
wrapper is immediately deallocated because it is not retained by anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. So we should retain the JSClosure instance itself.
* Refactor JSClosure to leverage FinalizationRegistry * Restore support for older runtimes using JSOneshotClosure + JSUnretainedClosure * Bump TS version * Fix build errors * Add escape hatch for old environments (#139) Co-authored-by: Yuta Saito <kateinoigakukun@gmail.com>
My only concern about #128 is dropping older versions of environments. The problem can be resolved easily and doesn't have much complexity.