Skip to content

Remove ConcurrentDictionary lookups from Unix socket event loop - #109052

Merged
MihaZupan merged 2 commits into
dotnet:mainfrom
MihaZupan:sockets-contextMap
Jan 14, 2025
Merged

MihaZupan merged 2 commits into
dotnet:mainfrom
MihaZupan:sockets-contextMap

Conversation

@MihaZupan

Copy link
Copy Markdown
Member

Running a bunch of different scenarios, this appears to be a slight improvement (<= 1%) on average, but that's below the run-to-run variance for these benchmarks.

@sebastienros is something like scenarios/platform.benchmarks.yml --scenario plaintext --profile aspnet-gold-lin reasonable for ASP.NET these days?

cc: @tmds @benaadams

@MihaZupan MihaZupan added this to the 10.0.0 milestone Oct 19, 2024
@MihaZupan
MihaZupan requested review from a team and antonfirsov October 19, 2024 19:49
@MihaZupan MihaZupan self-assigned this Oct 19, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@MihaZupan

Copy link
Copy Markdown
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs Outdated
@sebastienros

Copy link
Copy Markdown
Member

@MihaZupan

scenarios/platform.benchmarks.yml --scenario plaintext --profile aspnet-gold-lin

When working on overall perf gains we should use the gold machines now, correct. I would suggest to use JsonPlatform however for generic changes. Plaintext can be useful if you want to reduce the impact of networking (pipelining). I don't think we want that here.

@liveans liveans left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the offline chat about it!

@MihaZupan

Copy link
Copy Markdown
Member Author

The most consistent improvement I see is for platform json on citrine of just over 2%.
Other benchmarks are much closer to noise/within 1% diffs.

@MihaZupan
MihaZupan marked this pull request as draft December 3, 2024 11:35
@dotnet-policy-service dotnet-policy-service Bot removed this from the 10.0.0 milestone Jan 2, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@MihaZupan MihaZupan reopened this Jan 3, 2025
@MihaZupan

Copy link
Copy Markdown
Member Author

I switched the implementation from the non-standard use of "leaked" GCHandles to simply use an index into a global table as the data passed to epoll.

Running more benchmarks, this is showing a consistent ~3 % RPS improvement for platform Json on citrine.

@sebastienros

Copy link
Copy Markdown
Member

Running more benchmarks, this is showing a consistent ~3 % RPS improvement for platform Json on citrine.

There were recent changes in the runtime that triggered a similar improvement on JSON benchmark, please make sure you are only comparing your changes, ideally by rebasing on main or using the latest with your own builds of the changed and unchanged dlls.

@MihaZupan

Copy link
Copy Markdown
Member Author

I did, just manually replacing Sockets dlls

@antonfirsov antonfirsov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@MihaZupan
MihaZupan merged commit 00506eb into dotnet:main Jan 14, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants