-
Notifications
You must be signed in to change notification settings - Fork 280
Use Memory Stream for Serialization #2886
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
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.
Pull Request Overview
This PR addresses out of memory issues during column data serialization by replacing a direct JsonSerializer.Serialize/Deserialize pattern with a memory stream-based approach. The change aims to improve memory management during JSON processing of database result sets.
- Replaces direct string-based JSON serialization with memory stream approach
- Adds proper stream positioning and JsonDocument parsing
- Introduces resource management with using statements for memory cleanup
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
using MemoryStream ms = new(); | ||
using (Utf8JsonWriter writer = new(ms)) | ||
{ | ||
JsonSerializer.Serialize(writer, dbResultSetRow.Columns); | ||
} | ||
|
||
ms.Position = 0; | ||
using JsonDocument doc = JsonDocument.Parse(ms); |
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.
This approach may not resolve the original OOM issue and could introduce performance overhead. The MemoryStream still allocates memory internally, and you're now creating additional objects (MemoryStream, Utf8JsonWriter, JsonDocument). Consider using JsonSerializer.SerializeToUtf8Bytes() and JsonSerializer.Deserialize() with ReadOnlySpan for better memory efficiency.
using MemoryStream ms = new(); | |
using (Utf8JsonWriter writer = new(ms)) | |
{ | |
JsonSerializer.Serialize(writer, dbResultSetRow.Columns); | |
} | |
ms.Position = 0; | |
using JsonDocument doc = JsonDocument.Parse(ms); | |
byte[] jsonBytes = JsonSerializer.SerializeToUtf8Bytes(dbResultSetRow.Columns); | |
using JsonDocument doc = JsonDocument.Parse(jsonBytes); |
Copilot uses AI. Check for mistakes.
|
||
ms.Position = 0; | ||
using JsonDocument doc = JsonDocument.Parse(ms); | ||
JsonElement result = doc.RootElement.Clone(); |
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.
Please add a test that verifies the change works as expected :)
Why make this change?
We have recently seen an out of memory error coming from the line of code that serializes the column data.
What is this change?
Here, we are using a memory stream in the serialization process to avoid any out of memory errors.
There is a small performance overhead in using a memory stream - and I would like your opinion on whether that could be significant or not. The overhead could be negligible compared to allocating a large string, however, if you think performance would be an issue, we could change the code to only use the memory stream if the data is larger than a threshold
How was this tested?
Sample Request(s)