Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

Merged
j-f1 merged 5 commits into
finalization-registryswiftwasm/JavaScriptKit:finalization-registryfrom
katei/suggestion-pr-128swiftwasm/JavaScriptKit:katei/suggestion-pr-128Copy head branch name to clipboard
Sep 10, 2021
Merged

Suggestion for #128: Add escape hatch for old environments#139
j-f1 merged 5 commits into
finalization-registryswiftwasm/JavaScriptKit:finalization-registryfrom
katei/suggestion-pr-128swiftwasm/JavaScriptKit:katei/suggestion-pr-128Copy head branch name to clipboard

Conversation

@kateinoigakukun

@kateinoigakukun kateinoigakukun commented Sep 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

github-actions Bot commented Sep 9, 2021 •
edited
Loading

Copy link
Copy Markdown

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

MaxDesiatov
MaxDesiatov approved these changes Sep 9, 2021
View reviewed changes

@MaxDesiatov MaxDesiatov left a comment

Copy link
Copy Markdown
Member

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

kateinoigakukun commented Sep 9, 2021

Copy link
Copy Markdown
Member Author

Hmm, which line introduced performance regression... 🤔

@MaxDesiatov

MaxDesiatov commented Sep 9, 2021

Copy link
Copy Markdown
Member

Woah, yeah, that seems pretty serious

@kateinoigakukun

kateinoigakukun commented Sep 9, 2021

Copy link
Copy Markdown
Member Author

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

@MaxDesiatov

MaxDesiatov commented Sep 9, 2021

Copy link
Copy Markdown
Member

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

@kateinoigakukun

kateinoigakukun commented Sep 9, 2021

Copy link
Copy Markdown
Member Author

Here

JavaScriptKit/IntegrationTests/bin/benchmark-tests.js

Lines 25 to 40 in 43a33ee

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

MaxDesiatov commented Sep 9, 2021

Copy link
Copy Markdown
Member

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

kateinoigakukun commented Sep 9, 2021

Copy link
Copy Markdown
Member Author

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

@kateinoigakukun

kateinoigakukun commented Sep 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I couldn't see considerable bench regression on my end

@MaxDesiatov

MaxDesiatov commented Sep 9, 2021

Copy link
Copy Markdown
Member

Maybe it was a temporary CI glitch then?

j-f1
j-f1 reviewed Sep 9, 2021
View reviewed changes
Comment thread Runtime/src/index.ts Outdated
Comment thread Runtime/src/index.ts
Comment thread Runtime/src/index.ts Outdated
Comment thread Runtime/src/index.ts Outdated
Comment thread Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift Outdated
Comment thread Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift
// 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)

@j-f1 j-f1 Sep 9, 2021

Copy link
Copy Markdown
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.

@kateinoigakukun kateinoigakukun Sep 9, 2021

Copy link
Copy Markdown
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?

@j-f1 j-f1 Sep 10, 2021

Copy link
Copy Markdown
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.

@kateinoigakukun kateinoigakukun Sep 10, 2021

Copy link
Copy Markdown
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

@j-f1 j-f1 Sep 10, 2021

Copy link
Copy Markdown
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.

@kateinoigakukun kateinoigakukun Sep 10, 2021

Copy link
Copy Markdown
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.

Comment thread Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift Outdated
Comment thread Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift Outdated
Comment thread Sources/_CJavaScriptKit/_CJavaScriptKit.c
@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
@j-f1 @kateinoigakukun
Refactor JSClosure to leverage FinalizationRegistry (#128)
d987c3e 
* 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

Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@j-f1 j-f1 j-f1 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kateinoigakukun @MaxDesiatov @j-f1
Morty Proxy This is a proxified and sanitized view of the page, visit original site.