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

Convert sync over async network handling to async #835

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

msft-paddy14
Copy link
Contributor

@msft-paddy14 msft-paddy14 commented Nov 27, 2024

Currently Garnet's network processing in TLS mode happens by blocking the thread that receives the packet and it waits for TLS processing to complete. This leads to threadpool exhaustion and is an anti-pattern in general. It limits scalability where cores are limited as threads will get blocked and .NET will create more threads (where limit is not set). This makes things worst as memory footprint increases (~8MB per stack) and thread contention increases. We also convert Task to ValueTask for paths where async operation is already completed in general

BDN results for Network test added

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.2605) (Hyper-V)
AMD EPYC 7763, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100
[Host] : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2
.NET 8 : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2

Job=.NET 8 EnvironmentVariables=DOTNET_TieredPGO=0 Runtime=.NET 8.0
Server=True

Without changes (Main)

Method Params Mean Error StdDev Allocated
InlinePing None 1.964 μs 0.0040 μs 0.0033 μs -
Method Params Mean Error StdDev Allocated
Set None 14.85 μs 0.049 μs 0.046 μs -
SetEx None 19.87 μs 0.033 μs 0.029 μs -
SetNx None 18.17 μs 0.047 μs 0.039 μs -
SetXx None 19.00 μs 0.062 μs 0.055 μs -
GetFound None 15.95 μs 0.033 μs 0.031 μs -
GetNotFound None 11.33 μs 0.016 μs 0.014 μs -
Increment None 22.13 μs 0.078 μs 0.073 μs -
Decrement None 21.03 μs 0.045 μs 0.042 μs -
IncrementBy None 26.21 μs 0.069 μs 0.058 μs -
DecrementBy None 27.15 μs 0.064 μs 0.057 μs -

With Changes (current PR)

Job=.NET 8 EnvironmentVariables=DOTNET_TieredPGO=0 Runtime=.NET 8.0
Server=True

Method Params Mean Error StdDev Allocated
InlinePing None 2.008 μs 0.0096 μs 0.0090 μs -
Method Params Mean Error StdDev Allocated
Set None 14.17 μs 0.008 μs 0.007 μs -
SetEx None 19.74 μs 0.019 μs 0.017 μs -
SetNx None 17.78 μs 0.027 μs 0.025 μs -
SetXx None 18.41 μs 0.008 μs 0.007 μs -
GetFound None 15.48 μs 0.027 μs 0.024 μs -
GetNotFound None 11.09 μs 0.010 μs 0.009 μs -
Increment None 21.41 μs 0.126 μs 0.117 μs -
Decrement None 20.90 μs 0.037 μs 0.035 μs -
IncrementBy None 25.76 μs 0.063 μs 0.059 μs -
DecrementBy None 25.26 μs 0.025 μs 0.023 μs -

Remaining BDN Results

AsyncBDNWithChanges.log
MainBranch.log

@msft-paddy14 msft-paddy14 marked this pull request as ready for review December 1, 2024 09:31
@msft-paddy14 msft-paddy14 requested review from vazois and yrajas December 1, 2024 09:38
@TalZaccai TalZaccai requested a review from badrishc December 3, 2024 19:22
@badrishc badrishc requested review from ReubenBond and removed request for yrajas December 5, 2024 02:51
2. Refactor continuewith to async/await pattern
Copy link
Contributor

@vazois vazois left a comment

Choose a reason for hiding this comment

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

Looks good. Make sure to run the NetworkBDN and post the result vs main

@badrishc badrishc self-requested a review December 19, 2024 23:50
@badrishc
Copy link
Contributor

You can also increment the Version in version.props before merge so that we get a new release with this.

@badrishc badrishc requested review from badrishc and vazois December 20, 2024 11:23
Copy link
Contributor

@badrishc badrishc left a comment

Choose a reason for hiding this comment

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

In RawStringOps etc. the BDN calls should be async ... await as you had it earlier. We still need that else the BDN will return before the operation completes.

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