Performance improvements #202
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ardatan, 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 significantly refactors the GraphQL response serialization process by moving away from serde_json's preserve_order feature. It introduces a new, custom projection and serialization layer that directly constructs the JSON response string, ensuring field order based on the GraphQL selection set. This change aims to improve performance and maintain strict adherence to GraphQL response ordering specifications.
Highlights
- Custom GraphQL Response Serialization: Introduced a new
projectionmodule (lib/query-plan-executor/src/projection.rs) that handles the serialization of GraphQL response data directly into a JSON string. This new logic ensures that fields are ordered according to the GraphQL selection set, replacing the previous reliance onserde_json'spreserve_orderfeature. - Dependency Simplification: The
preserve_orderfeature has been removed fromserde_jsondependencies acrossbin/dev-cli,bin/gateway, andlib/query-plan-executor'sCargo.tomlfiles. This simplifies the dependency tree and removes the need forindexmapas a transitive dependency. - Performance Optimization: The new projection logic directly serializes the GraphQL response to a string, avoiding intermediate
serde_json::Valueallocations. This change is expected to improve the overall performance of response generation. - Refactored Execution Flow: The
execute_query_planfunction inlib/query-plan-executor/src/lib.rsnow returns the final serialized JSON string directly. Logic for addingqueryPlanextensions and handling introspection data has been integrated into the new projection mechanism, streamlining the response pipeline.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Federation Audit Results189 tests 189 ✅ 3s ⏱️ Results for commit 757a21c. ♻️ This comment has been updated with latest results. |
|
There was a problem hiding this comment.
Code Review
This pull request removes the preserve_order feature from serde_json and introduces custom serialization logic to ensure the GraphQL response fields are ordered according to the selection set.
The review identified several critical and high-severity issues in the new serialization code:
- Critical: The custom JSON serialization logic does not escape string values and object keys, which will lead to invalid JSON for certain inputs.
- High: There are several uses of
.unwrap()on serialization results, which could cause the gateway to panic and crash.
These issues should be addressed to ensure the stability and correctness of the gateway.
✅
|
GraphQL over HTTP Audit Results61 tests 61 ✅ 0s ⏱️ Results for commit 3279e86. ♻️ This comment has been updated with latest results. |
ardatan
force-pushed
the
no-preserve-order
branch
from
444ee6d to
3ac33e2
Compare
ardatan
changed the title
ardatan
force-pushed
the
no-preserve-order
branch
6 times, most recently
from
b4ca892 to
3279e86
Compare
ardatan
force-pushed
the
no-preserve-order
branch
3 times, most recently
from
151a9fc to
b728374
Compare
ardatan
force-pushed
the
no-preserve-order
branch
3 times, most recently
from
bdd3056 to
64dae35
Compare
This avoids allocating a new string for each intermediate result (same for vector).
The previous version was never reaching primitive cases because for primitives selection is empty already. This fixes that so it uses our serialization instead of serde_json.
…ions in project_requires (#211) Co-authored-by: Kamil Kisiela <kamil.kisiela@gmail.com>
Co-authored-by: Kamil Kisiela <kamil.kisiela@gmail.com>
Co-authored-by: Kamil Kisiela <kamil.kisiela@gmail.com>
ardatan
force-pushed
the
no-preserve-order
branch
from
2f95327 to
757a21c
Compare
#287) `HttpRequestParams` service clones stuff from `req` object which already has all the details. Instead we can extract those only when we need it from `req` using `trait`s as in this PR - Now PipelineError is created by a method of Request named `new_pipeline_error` that extract accept header inside. - PipelineErrorVariant -> PipelineError conversion is removed because it is error-prone to create a PipelineError without accept header. `req.new_pipeline_error` method is encouraged - Avoid copying accept header value, and check the accept header value in place
dotansimha
left a comment
dotansimha
left a comment
There was a problem hiding this comment.
Overall the changes in this PR are great and pushing us in the direction we wanted.
I would like to ask you to check if you can optimize and refactor the main lib.rs in this PR.
The main concern I have now is the fact that we have different way of handling the PlanNode variants in different cases (root? child? inside flatten?)
I feel like a better implementation will have to deal with this in a recursive/queue-link way, and deal with each kind of node just once. Currently, there are too many places where we have "special" handling, and it make the code messy.
3 participants
Closes Use serde_json::Map without preserve_order #201
Closes Use sonic-rs #215
PR Projection preparation to handle conflicting fields #285 & Bug why it is needed? Fix
circular-referenceaudit #249Closes Use io::Write (Vec<u8>) for JSON serialization #284