Skip to content
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

Suggestion for #128: Add escape hatch for old environments #139

Merged
merged 5 commits into from
Sep 10, 2021

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Sep 9, 2021

My only concern about #128 is dropping older versions of environments. The problem can be resolved easily and doesn't have much complexity.

@kateinoigakukun kateinoigakukun force-pushed the katei/suggestion-pr-128 branch 2 times, most recently from 5a7fed5 to 732b0a1 Compare September 9, 2021 13:42
@github-actions
Copy link

github-actions bot commented Sep 9, 2021

Time Change: -265.75ms (2%)

Total Time: 8,862.25ms

Test name Duration Change
Serialization/Write JavaScript number directly 204.75ms +23.75ms (11%) ⚠️
Serialization/Write JavaScript string directly 225.5ms +40ms (17%) ⚠️
ℹ️ View Unchanged
Test name Duration Change
Serialization/Swift Int to JavaScript 2,763.5ms -107.25ms (3%)
Serialization/Swift String to JavaScript 2,784.25ms -101ms (3%)
Object heap/Increment and decrement RC 2,884.25ms -121.25ms (4%)

performance-action

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a 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!

@kateinoigakukun
Copy link
Member Author

Hmm, which line introduced performance regression... 🤔

@MaxDesiatov
Copy link
Contributor

Woah, yeah, that seems pretty serious

@kateinoigakukun
Copy link
Member Author

The serious regression appeared only on JavaScript cases. So no regression in our codebase, I hope.

@MaxDesiatov
Copy link
Contributor

What are you referring to by "JavaScript cases" here?

@kateinoigakukun
Copy link
Member Author

Here

const serialization = new JSBenchmark("Serialization");
serialization.testSuite("Write JavaScript number directly", () => {
const jsNumber = 42;
const object = global;
for (let idx = 0; idx < 100; idx++) {
object["numberValue" + idx] = jsNumber;
}
});
serialization.testSuite("Write JavaScript string directly", () => {
const jsString = "Hello, world";
const object = global;
for (let idx = 0; idx < 100; idx++) {
object["stringValue" + idx] = jsString;
}
});

@MaxDesiatov
Copy link
Contributor

So you mean that FinalizationRegistry turns out to be about 20% slower when using it in Node.js than relying on manual deallocation, and there's nothing we can do in our codebase to make that go faster?

@kateinoigakukun
Copy link
Member Author

I guess that's right. But not sure why this regression didn't happen on the finalization-registry branch 🤔

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Sep 9, 2021

I couldn't see considerable bench regression on my end

@MaxDesiatov
Copy link
Contributor

Maybe it was a temporary CI glitch then?

Runtime/src/index.ts Outdated Show resolved Hide resolved
Runtime/src/index.ts Show resolved Hide resolved
Runtime/src/index.ts Outdated Show resolved Hide resolved
Runtime/src/index.ts Outdated Show resolved Hide resolved
Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift Outdated Show resolved Hide resolved
// 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)
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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.

Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift Outdated Show resolved Hide resolved
Sources/_CJavaScriptKit/_CJavaScriptKit.c Show resolved Hide resolved
@j-f1 j-f1 merged commit 033ff48 into finalization-registry Sep 10, 2021
@j-f1 j-f1 deleted the katei/suggestion-pr-128 branch September 10, 2021 14:59
kateinoigakukun added a commit that referenced this pull request Sep 10, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants