Improve performance of buffered string write#5985
Improve performance of buffered string write#5985bbowyersmyth wants to merge 2 commits intonpgsql:mainfrom
Conversation
|
/cc @NinoFloris |
NinoFloris
left a comment
NinoFloris
left a comment
There was a problem hiding this comment.
Thanks for the PR, it's a good optimization but it needs some tweaking before we can merge it.
| // Update PgSerializerOptions.IsWellKnownTextType(Type) after any changes to this list. | ||
| mappings.AddType<string>(DataTypeNames.Text, | ||
| static (options, mapping, _) => mapping.CreateInfo(options, new StringTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text), isDefault: true); | ||
| static (options, mapping, _) => mapping.CreateInfo(options, new StringTextConverter(options.TextEncoding, nested: false), preferredFormat: DataFormat.Text), isDefault: true); |
There was a problem hiding this comment.
Nestedness is not a mapping invariant. For instance this converter will be composed over by the array converter (via AddArrayType) and it would do the wrong thing. You'd have to look at the writer (we could maybe have some state there) to know whether the converter is called in a top-level context.
There was a problem hiding this comment.
Ok I'll take this PR back to draft for a while. I'll have to get a better understanding of the mapping to know what the next steps are.
| } | ||
|
|
||
| public void WriteChars(ReadOnlySpan<char> data, Encoding encoding) | ||
| public void WriteChars(ReadOnlySpan<char> data, Encoding encoding, int? encodedByteLength = null) |
There was a problem hiding this comment.
I don't think we should make these error-prone methods public. Instead we can apply the optimization to the internal converters. I'm fine having these signatures accessible internally.
|
I'm going to close this draft. The idea is sound, but the approach has some problems. If we want to avoid the extra work we should instead return a writeState from GetSize that contains the encoded length, this would work in all cases. The downside being that it adds an alloc per string write. |
3 participants
When the string to be written can potentially be fully buffered there is a second call to
GetByteCount(first one is GetSize). If the converter is not nested inside another converter, the size has already been calculated.Basing this off BitArrayBitStringConverter that its safe to do.
PgReader.cs change is just to remove the DisplayClass struct creation.
Text.cs test string of 10_000 doesn't fit in the test buffer.
Before
After