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

Async implementation #101

Merged
merged 16 commits into from
Jan 16, 2025
Merged

Conversation

cmcknight-bb
Copy link
Contributor

Hello again, as promised I have made an attempt to implement async over the current uniffi-rs fork version. It seems to be working for our internal poc, so I thought it was a good time to contribute upstream and get some eyes on it. As discussed, rust pointers needed to be marshalled to IntPtr instead of SafeHandle. Performance seems to be ok, I stole the async tests from the kristupas/async-0.24 branch and they pass most of the time, but can be flaky when they do not quite meet the tolerances. Let me know if I need to clarify anything, hope everyone had a great a holiday!

Copy link
Contributor

@arg0d arg0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, it looks like the async performance issues are solved in this PR. I still need a few days to finish reviewing the changes, but this looks good at first glance!

bindgen/templates/Async.cs Outdated Show resolved Hide resolved
@cmcknight-bb cmcknight-bb force-pushed the cmcknight-bb/async-bindings branch from 38f0506 to 29a4dbb Compare January 8, 2025 20:28
Copy link
Contributor

@arg0d arg0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The async code looks great, it's neat and straight to the point. I left some comments about object synchronization code, and some other minor comments.

I'm more concerned with the performance numbers. It seems like futures can only handle quite small number of calls with scheduling, ~3,000/s. I also tested with uniffi Kotlin, and I'm getting ~5,000/s. Maybe you could also test futures performance on your end? And let me know what kind of performance you are expecting from futures.

If we deem the performance adequate, I think this would be OK to merge after other comments are resolved.

bindgen/templates/ObjectRuntime.cs Outdated Show resolved Hide resolved
bindgen/templates/ObjectTemplate.cs Outdated Show resolved Hide resolved
bindgen/templates/ObjectTemplate.cs Outdated Show resolved Hide resolved
bindgen/templates/TopLevelFunctionTemplate.cs Outdated Show resolved Hide resolved
bindgen/templates/TopLevelFunctionTemplate.cs Outdated Show resolved Hide resolved
bindgen/templates/Types.cs Outdated Show resolved Hide resolved
bindgen/templates/wrapper.cs Outdated Show resolved Hide resolved
bindgen/src/gen_cs/mod.rs Outdated Show resolved Hide resolved
bindgen/templates/ObjectRuntime.cs Outdated Show resolved Hide resolved
@cmcknight-bb cmcknight-bb force-pushed the cmcknight-bb/async-bindings branch 2 times, most recently from 0d5e7a8 to be1c6e5 Compare January 14, 2025 17:55
bindgen/templates/Async.cs Show resolved Hide resolved
Copy link
Contributor

@arg0d arg0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of suggestions how to deal with the flaky tests TestAsyncFallibleFunctions and TestBrokenSleep. Considering that you deem the async performance acceptable, I think it's enough to raise the timing tolerances slightly.

dotnet-tests/UniffiCS.BindingTests/TestFutures.cs Outdated Show resolved Hide resolved
@cmcknight-bb
Copy link
Contributor Author

I left a couple of suggestions how to deal with the flaky tests TestAsyncFallibleFunctions and TestBrokenSleep. Considering that you deem the async performance acceptable, I think it's enough to raise the timing tolerances slightly.

Committed these changes and flaky tests seem fixed on my end.

I was using the following decorators to disable parallelization for performance testing. I can commit them if you think it would help stability. Just didn't want to conflict with the other open PR.

[Collection(nameof(TestFuturesCollection))]
public class TestFutures
{
...
}

[CollectionDefinition(nameof(TestFuturesCollection), DisableParallelization = true)]
public class TestFuturesCollection { }

cmcknight-bb and others added 16 commits January 15, 2025 12:43
Signed-off-by: Chloe McKnight <[email protected]>
Co-authored-by: Kristupas Antanavicius <[email protected]>
Signed-off-by: Chloe McKnight <[email protected]>
Co-authored-by: Kristupas Antanavicius <[email protected]>
Signed-off-by: Chloe McKnight <[email protected]>
Signed-off-by: Chloe McKnight <[email protected]>
Signed-off-by: Chloe McKnight <[email protected]>
@cmcknight-bb cmcknight-bb force-pushed the cmcknight-bb/async-bindings branch from 3405372 to f939e35 Compare January 15, 2025 18:52
Copy link
Contributor

@arg0d arg0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging this, thanks for the contribution!

@arg0d arg0d merged commit 3460be3 into NordSecurity:main Jan 16, 2025
3 checks passed
@cmcknight-bb cmcknight-bb deleted the cmcknight-bb/async-bindings branch January 16, 2025 16:11
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.

2 participants