Relieve GC and memory pressure for typical workloads in ClickHouseDataReader #6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ory usage for small to medium result sets by replacing BufferedStream with RecyclableMemoryStream in ClickHouseDataReader�Every query allocated a new 512KB buffer via BufferedStream, regardless of actual response size, creating allocation and GC pressure for typical workloads.
|
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes memory usage in ClickHouseDataReader by replacing the buffered stream approach with pooled RecyclableMemoryStream instances to reduce GC pressure and improve performance for typical workloads.
- Switches from
BufferedStreamwith fixed 512KB allocation toRecyclableMemoryStreamManagerwith configurable pooling - Adds comprehensive benchmarking infrastructure with comparison tests between old and new implementations
- Updates target framework from .NET 8.0 to .NET 9.0 for benchmark and integration test projects
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs |
Core implementation change replacing BufferedStream with RecyclableMemoryStream |
ClickHouse.Driver/Types/TypeConverter.cs |
Added InternalsVisibleTo attribute for benchmark project access |
ClickHouse.Driver.Benchmark/RecyclableMemoryStreamBenchmark.cs |
New benchmark comparing BufferedStream vs RecyclableMemoryStream performance |
ClickHouse.Driver.Benchmark/References/BufferedStreamClickHouseDataReader.cs |
Reference implementation of original BufferedStream approach for benchmarking |
ClickHouse.Driver.Benchmark/ClickHouse.Driver.Benchmark.csproj |
Target framework update to .NET 9.0 |
ClickHouse.Driver.IntegrationTests/ClickHouse.Driver.IntegrationTests.csproj |
Target framework update to .NET 9.0 |
run-benchmarks.ps1 |
PowerShell script for running memory stream benchmarks |
run-tests.ps1 |
PowerShell script for test execution with Docker ClickHouse setup |
| private readonly HttpResponseMessage httpResponse; // Used to dispose at the end of reader | ||
| private readonly ExtendedBinaryReader reader; | ||
| private readonly RecyclableMemoryStream recyclableStream; | ||
|
|
There was a problem hiding this comment.
The constructor signature has changed by adding a RecyclableMemoryStream parameter. This is a breaking change to the internal API that could affect code that directly instantiates this class, even though it's marked as internal.
Suggested change
| // Backward-compatible constructor overload (without RecyclableMemoryStream) | |
| private ClickHouseDataReader(HttpResponseMessage httpResponse, ExtendedBinaryReader reader, string[] names, ClickHouseType[] types) | |
| : this(httpResponse, reader, null, names, types) | |
| { | |
| } |
| { | ||
| var stream = new BufferedStream(httpResponse.Content.ReadAsStreamAsync().GetAwaiter().GetResult(), BufferSize); | ||
| reader = new ExtendedBinaryReader(stream); // will dispose of stream | ||
| recyclableStream = StreamManager.GetStream("ClickHouseDataReader"); |
There was a problem hiding this comment.
The string literal "ClickHouseDataReader" should be extracted to a constant to avoid magic strings and ensure consistency if used elsewhere.
Suggested change
| recyclableStream = StreamManager.GetStream("ClickHouseDataReader"); | |
| recyclableStream = StreamManager.GetStream(StreamName); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using the buffered stream approach in
ClickHouseDataReaderallocates 512KB for each usage, regardless of payload size.This change switches to using pooled
RecyclableMemoryStreaminstances, removing GC pressure for more frequent queries, with good characteristics for small to medium result sets, and comparable performance <50000 rows.Benchmark results are below.
@SpencerTorres