-
Notifications
You must be signed in to change notification settings - Fork 6
Refactor JSON string writing and escaping logic #638
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @kamilkisiela, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request overhauls the internal mechanism for writing and escaping JSON strings, adopting a high-performance, SIMD-optimized solution. The goal is to boost the efficiency of JSON serialization within the executor. Additionally, it refines the estimation of final response sizes to better reflect the actual memory footprint after JSON formatting. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant performance improvement by replacing the existing JSON string escaping logic with a SIMD-accelerated implementation from sonic-rs. While the performance gain is valuable, the new code introduces a large amount of unsafe Rust. My review focuses on ensuring this new code adheres to the project's style guide, particularly regarding safety, and improving the maintainability of the new tests and heuristics.
✅
|
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-72488c6f-a481-4923-abba-9d619aca6464/builder-72488c6f-a481-4923-abba-9d619aca64640/sgvymujd983npxn317rlvp061",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:9fb1d1e6f84daa1a53aec39a9168ee3c29c2b07640d6981294eb0c4b7adbf439",
"size": 1609
},
"containerimage.digest": "sha256:9fb1d1e6f84daa1a53aec39a9168ee3c29c2b07640d6981294eb0c4b7adbf439",
"image.name": "ghcr.io/graphql-hive/router:pr-638,ghcr.io/graphql-hive/router:sha-cc9b7e7"
} |
Added a buffer to the final response size estimate.
Updated the version of the sonic-simd package from 0.1.1 to 0.1.2 and added new dependencies.
Updated documentation link in json_writer.rs
8a88d31 to
8d0d7a8
Compare
This PR significantly improves JSON response serialization (response projection) performance (50% faster) by replacing the existing character-by-character string escaping logic with a SIMD-accelerated implementation adapted from sonic-rs.
I also improved
estimate_final_response_size()with 20% buffer for JSON overhead, that according to the tests I did and responses in the federation audit, is a good estimate.This code is complex and challenging to maintain, due to the low-level SIMD operations, unsafe pointer manipulation, and platform-specific optimizations. However, the previous implementation was also difficult to maintain in its own way.
The key advantage here is that we're leveraging battle-tested code from
sonic-rs, a library we already depend on indirectly.