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

Remove the unsafe code from Kestrel #4720

Open
davidfowl opened this issue Sep 15, 2018 · 9 comments
Open

Remove the unsafe code from Kestrel #4720

davidfowl opened this issue Sep 15, 2018 · 9 comments
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel Perf severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@davidfowl
Copy link
Member

This is more of an aspirational goal instead of something we would immediately do but we should have enough primitives in the BCL now that this is viable. Of course there might be a tiny performance hit but it might be worth it if we can remove all of the unsafe code.

cc @DamianEdwards @blowdart @halter73 @KrzysztofCwalina @benaadams

@davidfowl davidfowl changed the title Remove Unsafe code from Kestrel Removeunsafe code from Kestrel Sep 15, 2018
@davidfowl davidfowl changed the title Removeunsafe code from Kestrel Remove the unsafe code from Kestrel Sep 15, 2018
@benaadams
Copy link
Member

Be nice to do it and not get a performance hit. For example if the Jit could recognise slice advancing and propagate bounds checking; as well as elide slice bounds checking and secondary span creation so that; in the correct pattern, it could be as near cost to a pointer bump

@blowdart
Copy link
Contributor

@benaadams don't take this away from me

@KrzysztofCwalina
Copy link
Member

@davidfowl, do you consider the http parser to be "kestrel"? If yes, do you think we can remove unsafe code from the parser?

@davidfowl
Copy link
Member Author

davidfowl commented Sep 17, 2018

@KrzysztofCwalina yes and yes. I had a branch here in the 2.0 time frame before JIT optimizations on span and the performance hit was too big https://github.com/aspnet/KestrelHttpServer/tree/davidfowl/ideal-parser. I need to go back to it now that those things were addressed.

@aspnet-hello aspnet-hello transferred this issue from aspnet/KestrelHttpServer Dec 13, 2018
@aspnet-hello aspnet-hello added this to the 3.1.0 milestone Dec 13, 2018
@analogrelay analogrelay modified the milestones: 3.1.0, 3.0.0-preview8, Backlog Jun 4, 2019
@jkotalik jkotalik added affected-very-few This issue impacts very few customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-nice-to-have This label is used by an internal tool labels Nov 13, 2020 — with ASP.NET Core Issue Ranking
@davidfowl davidfowl added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Mar 28, 2021
@davidfowl
Copy link
Member Author

This is a long running issue. We should be able to track this somehow.

@davidfowl
Copy link
Member Author

Actually, it would be great if we could quantify this and drive it down.

@BrennanConroy
Copy link
Member

List all currently known methods as a checklist?

@davidfowl
Copy link
Member Author

Yep

@GrabYourPitchforks
Copy link
Member

Folks working on this may be interested in the conversation taking place at dotnet/runtime#41418. That issue attempts to catalog the various MemoryMarshal and related APIs as safe vs. unsafe-equivalent.

tl;dr: if you're removing use of unsafe in the spirit of reliability or security hardening, avoid the unsafe-equivalent APIs listed in the table in that issue, otherwise your work likely won't result in any measurable hardening.

@BrennanConroy BrennanConroy removed the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 12, 2021
@jkotas jkotas mentioned this issue Dec 28, 2021
4 tasks
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel Perf severity-nice-to-have This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

10 participants