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

Improve FastParseCommand readability #119

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

aromaa
Copy link
Contributor

@aromaa aromaa commented Mar 25, 2024

Modified the code to use switch expression with pattern matching.

@aromaa
Copy link
Contributor Author

aromaa commented Mar 25, 2024

@microsoft-github-policy-service agree

@aromaa
Copy link
Contributor Author

aromaa commented Mar 25, 2024

Failures seem to be unrelated, other PRs are failing with same tests related to the TLS handshake.

@aromaa
Copy link
Contributor Author

aromaa commented Mar 26, 2024

Was actually able to find small workaround for the small regression on .NET 6. This is now a perf win all across the board.

@lmaas
Copy link
Contributor

lmaas commented Mar 28, 2024

Great job! Thank you for your diligent work on this. There's definitely a lot of room for performance optimization in the parsing loop, and your approach seems spot-on.

However, I am holding off on merging this pull request for now. We're currently in the process of refactoring the parsing logic, and we have made changes to the semantics of the FastParseCommand() function (see PR #164). The new version you've proposed aligns well with the new interface, and I appreciate the improved readability.

If you have the time, I’d appreciate it if you could review the PR and provide any comments. Once #164 is merged, we can proceed with integrating this one.

@lmaas
Copy link
Contributor

lmaas commented Mar 28, 2024

PS: Thanks also for the benchmark! This is super useful. Once we have command parsing and execution separated, we should think about adding a proper parsing benchmark that follows your example.

@aromaa
Copy link
Contributor Author

aromaa commented Mar 28, 2024

I see, glad someone was already working on larger clean up on this. Looks like we had similar ideas already. Seems to be a very nice clean up :)

@aromaa aromaa changed the title Improve FastParseCommand performance & readability Improve FastParseCommand readability Apr 3, 2024
@aromaa
Copy link
Contributor Author

aromaa commented Apr 3, 2024

Updated the PR on top of main as #164 is merged now. I didn't touch any of the FastParseArrayCommand as the mileage varies on how much benefit you get from there with this pattern.

Copy link
Contributor

@lmaas lmaas left a comment

Choose a reason for hiding this comment

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

Great work! The code looks solid overall and the performance looks good as well. I noticed two minor typos in the comments. Once those are fixed, I'll be happy to merge this PR. Thank you for your contribution! 🙌

libs/server/Resp/RespCommand.cs Outdated Show resolved Hide resolved
@lmaas lmaas merged commit 50a0d56 into microsoft:main Apr 3, 2024
21 checks passed
@aromaa aromaa deleted the improve-resp-commands branch April 4, 2024 12:22
altall pushed a commit to altall/garnet that referenced this pull request Apr 5, 2024
* Improve FastParseCommand readability

* PR feedback

---------

Co-authored-by: Lukas Maas <lumaas@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants