perf: drop the pool dance from MemorySliceLineSource.Concat (#119)#124
Merged
Conversation
ConcatenateMemory (and its post-#118 successor MemorySliceLineSource.Concat) rented a buffer from CharArrayPool, copied the three input segments into it, then immediately allocated a fresh char[] of exact size, copied everything again into that array, and returned the rented buffer to the pool. Two allocations and three extra copies where one allocation would have done. Replace with `string.Concat(head.Span, newLine.AsSpan(), tail.Span)`, which performs a single contiguous allocation (no pool churn, no second copy). Also capture the materialized string into the `out string? combined` parameter, matching the other ILineSource implementations -- the OptimizedRowFactory ignores it today, but the cost of publishing it is zero now that we've already created the string. The unused CsvMemoryOptions field and constructor parameter on MemorySliceLineSource are dropped; CsvReader.ReadFromMemoryOptimizedImpl updated accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request simplifies the Concat method within the MemorySliceLineSource struct by replacing manual buffer pooling and copying logic with a call to string.Concat. As a result, the CsvMemoryOptions dependency has been removed from the struct and its constructor. I have no feedback to provide as there were no review comments.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
ConcatenateMemory(and its post-RFC: Unify the four CsvReader read loops behind one JIT-devirtualized engine #118 successorMemorySliceLineSource.Concat) rented a buffer fromCharArrayPool, copied the three input segments in, then immediately allocated a freshchar[]of exact size and copied again — defeating the pool entirely and paying two allocations + three copies where one allocation suffices.string.Concat(head.Span, newLine.AsSpan(), tail.Span). Single contiguous allocation, no pool churn, no second copy. Materialized string is captured intoout string? combinedso it's not discarded.CsvMemoryOptionsfield and constructor parameter onMemorySliceLineSource;CsvReader.ReadFromMemoryOptimizedImplupdated accordingly.Resolves #119.
Allocation profile
new char[totalLength]+ 3CopyTostring.Concat(one alloc, internal contiguous copy)Net: one fewer allocation, two fewer copies, no
ArrayPooloverhead, on every multiline continuation step ofReadFromMemoryOptimized.Notes
MemoryReaderLineSource.Concatalready usedStringHelpers.Concat(which internally callsstring.Concat); onlyMemorySliceLineSourcehad the anti-pattern.OptimizedRowFactory.Createstill ignoresrawString, so the capturedcombinedstring isn't consumed downstream — but the cost is zero now that we've materialized it anyway.Test plan
dotnet buildclean on netstandard2.0, net8.0, net9.0dotnet test— 169/169 passing (excluding the pre-existing flakyMemory_AllocationComparisonGC-ratio test)Csv.Tests/EngineUnificationTests.cscover theReadFromMemoryOptimizedmultiline path; they continue to pass without modification🤖 Generated with Claude Code