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

Migrate to use Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets #387

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Migrate to use Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets #387

merged 3 commits into from
Aug 24, 2023

Conversation

TheVeryStarlk
Copy link
Contributor

Currently Obsidian uses the TcpListener for the server and raw Socket for the client. While those classes do the job, the issue #96 raises a concern about performance due to the fact that the currently used classes do not make use of the optimized ways for data receiving and sending. I benchmarked the login state sequence, before using the PR and after. The server averaged at 841140 ticks after the PR, and 934719 ticks before. (Results are not 100% accurate), it can also be significantly improved if we start using System.IO.Pipelines, and this PR is a step towards that.

Jonpro03
Jonpro03 previously approved these changes Aug 22, 2023
Copy link
Member

@Jonpro03 Jonpro03 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 to me, but need more 👀 for sure

Obsidian/Client.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@InFTord InFTord left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Naamloos Naamloos left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I don't see any issues so far.

@Naamloos Naamloos merged commit 1d6b40b into ObsidianMC:master Aug 24, 2023
1 of 2 checks passed
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.

4 participants