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

Refactor the SpanByte usage #244

Merged
merged 14 commits into from
Apr 8, 2024

Conversation

PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented Apr 5, 2024

This PR brings in the good changes from microsoft/FASTER#903 and renames some terminology around "fixed" buffers to "pinned". The latter term is more commonly used in .NET APIs and avoids the problem with possibly getting mixed with fixed sized buffers.

Also marked some of the SpanByte(AndMemory) members readonly and removed some unnecessary methods.

Also where is Reinterpret overloads supposed to be used? Nvm, just saw the command parsing logic using it 👍

@PaulusParssinen PaulusParssinen changed the title Spanbytes from pinned Refactor the methods expecting pinned buffers Apr 5, 2024
@PaulusParssinen PaulusParssinen changed the title Refactor the methods expecting pinned buffers Refactor the SpanByte methods expecting pinned buffers Apr 5, 2024
@PaulusParssinen PaulusParssinen changed the title Refactor the SpanByte methods expecting pinned buffers Refactor the SpanByte usage Apr 5, 2024
@PaulusParssinen PaulusParssinen marked this pull request as ready for review April 5, 2024 14:25
@badrishc
Copy link
Contributor

badrishc commented Apr 5, 2024

Folks, everything you see in the code was there for a reason. Either evolution over many years, workarounds that were necessary at some point but probably rendered unnecessary as .NET evolved, or careful micro benchmarking that necessitated the non traditional styles. It was not easy to get 100 million plus ops/sec with low latency variance that beats the simple C code of systems like Redis even on a single thread. Can it be revisited? Sure, and we welcome it, including all the contributions to leverage the latest language speedups. Should we have delayed open sourcing by 3-6 months just for syntactic happiness from purists? I think not.

@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented Apr 5, 2024

Folks, everything you see in the code was there for a reason. Either evolution over many years, workarounds that were necessary at some point but probably rendered unnecessary as .NET evolved, or careful micro benchmarking that necessitated the non traditional styles. It was not easy to get 100 million plus ops/sec with low latency variance that beats the simple C code of systems like Redis even on a single thread. Can it be revisited? Sure, and we welcome it, including all the contributions to leverage the latest language speedups. Should we have delayed open sourcing by 3-6 months just for syntactic happiness from purists? I think not.

I apologize for my loaded wording in the comments. It's not directed at you or anyone in the team in particular but more towards the general problem of extermely complex nature of managing low-level .NET code. There's certain APIs in .NET that can cause pitfalls for even most experienced .NET developers and Unsafe.AsPointer is one of them. There are instances of .NET team members misusing it and it signals that the API is truly to be avoided. It also seems there has not been enough communication of all the dangers of that API from documentation perspective.. This project also has exposed some areas where .NET in general could improve to close gap between pure pointer code and equivalent .NET constructs which is a good thing.

Once again, sorry for the inappropiate comment. It's easy for me criticize from hindsight. I have huge respect for the thing you guys have done here as the code has lived over the years and .NET has evolved at the same time. I want to help make this thing safer while maintaining the performance with modern .NET.

edit: Also noting that there are legitimate concerns here and not to be dismissed as just syntactic purism, the intermittent random crashes caused by GC holes are not fun to try track down. However we have issue tracking those already 👍

@badrishc
Copy link
Contributor

badrishc commented Apr 6, 2024

Thanks for your perspective! I'm excited for the next few months as we explore scaling back some of the unsafe code without compromising performance. Your stellar PRs are definitely much appreciated, and folks on our team are actively learning and looking at ways to improve and microbenchmark as well.

@vazois vazois merged commit 763c517 into microsoft:main Apr 8, 2024
28 checks passed
@PaulusParssinen PaulusParssinen deleted the spanbytes-from-pinned branch April 8, 2024 19:56
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants